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

Fire 'select' event in SetSelection{Start,End} #19202

Merged
merged 1 commit into from Nov 15, 2017

Conversation

Projects
None yet
6 participants
@jonleighton
Contributor

jonleighton commented Nov 13, 2017

See #19171.


This change is Reviewable

@highfive

This comment has been minimized.

highfive commented Nov 13, 2017

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @emilio (or someone else) soon.

@highfive

This comment has been minimized.

highfive commented Nov 13, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/htmlinputelement.rs
  • @KiChjang: components/script/dom/htmlinputelement.rs
@tigercosmos

tigercosmos requested changes Nov 13, 2017 edited

You can delete the pass test cases in tests/wpt/metadata/html/semantics/forms/textfieldselection/selection-start-end.html.ini
We only need to remain the unexpected ones

[onselect should fire when selectionStart is changed on input-appended-prefocused]
expected: NOTRUN
expected: PASS

This comment has been minimized.

@tigercosmos

tigercosmos Nov 13, 2017

Collaborator

If Pass, You can delete it directly

@KiChjang

👍 Great start! I only have minor comments about adding comments ;). Once you've addressed them, you can r=me.

self.textinput.borrow().selection_direction
}
fn set_selection_range(&self, start: u32, end: u32, direction: SelectionDirection) {

This comment has been minimized.

@KiChjang

KiChjang Nov 14, 2017

Member

Let's add a spec link comment above this function (https://html.spec.whatwg.org/multipage/#set-the-selection-range).

self.textinput.borrow_mut().selection_direction = direction;
self.textinput.borrow_mut().set_selection_range(start, end);
let window = window_from_node(self);
let _ = window.user_interaction_task_source().queue_event(

This comment has been minimized.

@KiChjang

KiChjang Nov 14, 2017

Member

Let's also add step annotations for this function. For example, this particular line here follows step 6 of the linked algorithm (albeit incorrectly).

@jonleighton jonleighton force-pushed the jonleighton:issue-19171 branch from 45f4483 to f135348 Nov 15, 2017

@jonleighton

This comment has been minimized.

Contributor

jonleighton commented Nov 15, 2017

Thanks, requested changes implemented

@jonleighton

This comment has been minimized.

Contributor

jonleighton commented Nov 15, 2017

@highfive highfive assigned KiChjang and unassigned emilio Nov 15, 2017

@KiChjang

One final nit!

@@ -1,6 +1,6 @@
[selection-start-end.html]
type: testharness
expected: TIMEOUT
expected: OK

This comment has been minimized.

@KiChjang

KiChjang Nov 15, 2017

Member

You can remove this line now.

@KiChjang

This comment has been minimized.

Member

KiChjang commented Nov 15, 2017

Also, after you've made the changes, can you squash your commits into one, please?

@jonleighton jonleighton force-pushed the jonleighton:issue-19171 branch from f135348 to 93b047a Nov 15, 2017

@jonleighton

This comment has been minimized.

Contributor

jonleighton commented Nov 15, 2017

Sure, done

@KiChjang

This comment has been minimized.

Member

KiChjang commented Nov 15, 2017

@bors-servo r+

Thanks!

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 15, 2017

📌 Commit 93b047a has been approved by KiChjang

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 15, 2017

⌛️ Testing commit 93b047a with merge 2efbf22...

bors-servo added a commit that referenced this pull request Nov 15, 2017

Auto merge of #19202 - jonleighton:issue-19171, r=KiChjang
Fire 'select' event in SetSelection{Start,End}

See #19171.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19202)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 15, 2017

@bors-servo bors-servo merged commit 93b047a into servo:master Nov 15, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment