Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement 'ask for a reset' algorithm for <select> elements #7774

Closed
frewsxcv opened this issue Sep 28, 2015 · 8 comments
Closed

Implement 'ask for a reset' algorithm for <select> elements #7774

frewsxcv opened this issue Sep 28, 2015 · 8 comments

Comments

@frewsxcv
Copy link
Member Author

@frewsxcv frewsxcv commented Sep 28, 2015

Relevant FIXME which was added in #7745

@dagnir
Copy link
Contributor

@dagnir dagnir commented Oct 4, 2015

I'm completely new to servo (and any browser engine dev). From looking at the spec, this seems relatively simple. Is this appropriate for a newcomer?

@jdm
Copy link
Member

@jdm jdm commented Oct 4, 2015

I believe so! You'll probably want to implement HTMLSelectElement's size attribute, followed by the multiple attribute, (both using appropriate macros from macros.rs) and then add a ask_for_reset method. Let us know if anything is unclear!

@jdm
Copy link
Member

@jdm jdm commented Oct 4, 2015

Looks like the multiple attribute already is implemented: http://mxr.mozilla.org/servo/source/components/script/dom/htmlselectelement.rs#82

@dagnir
Copy link
Contributor

@dagnir dagnir commented Oct 4, 2015

Thank you. I'll get to work on it

@frewsxcv frewsxcv added the C-assigned label Oct 4, 2015
@dagnir
Copy link
Contributor

@dagnir dagnir commented Oct 9, 2015

Hi @jdm, here is what I've put together so far dagnir@c6267df. Please let me know what things need fixing before I send a PR

@jdm
Copy link
Member

@jdm jdm commented Oct 9, 2015

A few useful changes:

  • instead of calling Item in a loop, we should be using traverse_preorder and iterating through the results
  • uses of match that only act on the Some branch can be written as if let Some( instead
  • uses of if something.is_some() { something.unwrap() } can be written as if let Some( instead
  • first_enabled and last_selected can probably be Option<Root<HTMLOptionElement>> instead of indexes
  • unfortunately the Disabled method is not quite enough; we need to call get_disabled_state instead

Generally it's easier to make comments like these in our review tool; submitting PRs for early feedback is encouraged :)

@dagnir
Copy link
Contributor

@dagnir dagnir commented Oct 9, 2015

@jdm Thanks! I'lll send PRs in the future

dagnir added a commit to dagnir/servo that referenced this issue Oct 10, 2015
dagnir added a commit to dagnir/servo that referenced this issue Oct 10, 2015
dagnir added a commit to dagnir/servo that referenced this issue Oct 18, 2015
dagnir added a commit to dagnir/servo that referenced this issue Oct 18, 2015
dagnir added a commit to dagnir/servo that referenced this issue Oct 20, 2015
dagnir added a commit to dagnir/servo that referenced this issue Oct 23, 2015
bors-servo added a commit that referenced this issue Oct 28, 2015
Implement ask_for_reset for HTMLSelectElement.

Fixes #7774

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7963)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.