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

Further changes in relation to #19171 #19272

Merged
merged 2 commits into from Nov 21, 2017

Conversation

Projects
None yet
6 participants
@jonleighton
Contributor

jonleighton commented Nov 17, 2017

This change is Reviewable

@highfive

This comment has been minimized.

highfive commented Nov 17, 2017

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

@highfive

This comment has been minimized.

highfive commented Nov 17, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/htmltextareaelement.rs, components/script/dom/textcontrol.rs, components/script/dom/mod.rs, components/script/dom/htmlinputelement.rs
  • @KiChjang: components/script/dom/htmltextareaelement.rs, components/script/dom/textcontrol.rs, components/script/dom/mod.rs, components/script/dom/htmlinputelement.rs
@jonleighton

This comment has been minimized.

Contributor

jonleighton commented Nov 17, 2017

@highfive highfive assigned KiChjang and unassigned asajeffrey Nov 17, 2017

@jonleighton

This comment has been minimized.

Contributor

jonleighton commented Nov 17, 2017

I don't know if this TextControl trait is the right abstraction, so feedback on that front is very welcome. It kind of annoys me that now there's lots of boilerplate code to call in to the methods defined by TextControl.

@KiChjang

This comment has been minimized.

Member

KiChjang commented Nov 17, 2017

This is a larger change than I expected, and I'm not comfortable reviewing this. r? @jdm

@highfive highfive assigned jdm and unassigned KiChjang Nov 17, 2017

@jdm jdm added the S-fails-tidy label Nov 17, 2017

@jdm

This seems like a sensible solution. The only other abstraction I came up with is free functions that accept the textinput as an argument, rather than putting them all in a trait, but I don't see a compelling reason to use that design instead.

@@ -59,10 +59,18 @@
assert_equals(testValue.length, 10);
}, "Sanity check for testValue length; if this fails, variou absolute offsets in the test below need to be adjusted to be less than testValue.length");
for (let prop of ["selectionStart", "selectionEnd"]) {
test(function() {

This comment has been minimized.

@jdm

jdm Nov 17, 2017

Member

This outer test is not necessary.

This comment has been minimized.

@jonleighton

jonleighton Nov 18, 2017

Contributor

Thanks. I thought that maybe it was a bad idea to call createTestElements outside of a test, but perhaps it doesn't actually matter.

@jdm

This comment has been minimized.

Member

jdm commented Nov 17, 2017

Also ./mach test-tidy fails with these changes.

// https://html.spec.whatwg.org/multipage/#dom-textarea/input-selectionstart
fn dom_selection_start(&self) -> u32 {
self.textinput().borrow().get_selection_start()

This comment has been minimized.

@KiChjang

KiChjang Nov 17, 2017

Member

When I mentioned that whether the selection{Start, End, Range} functions matches the spec, I meant whether they are following the steps as defined in the spec. This function in particular does not seem to have included step 1 and 2.

@KiChjang

This comment has been minimized.

Member

KiChjang commented Nov 17, 2017

Also, may I ask why this abstraction is necessary? It seems to me that the intent is to generalize the textinput stuff across HTMLInputElement and HTMLTextAreaElement, but I may be wrong on that.

@jonleighton

This comment has been minimized.

Contributor

jonleighton commented Nov 18, 2017

The only other abstraction I came up with is free functions that accept the textinput as an argument

Out of curiosity, can you point me to anything which describes what is meant by a "free function"? (I'm not very familiar with Rust yet...)

Also ./mach test-tidy fails with these changes.

I'll look into that.

When I mentioned that whether the selection{Start, End, Range} functions matches the spec, I meant whether they are following the steps as defined in the spec. This function in particular does not seem to have included step 1 and 2.

Yeah, I'm not done yet but I thought it was worth submitting this PR to get feedback before I make further changes. For now, the implementation of TextControl is simply moved from elsewhere in the code base. I'd then intend to work through the spec to make sure it conforms to everything. (I can continue working on the subsequent changes if desired, but I just wanted to get some feedback so I don't go too far down an undesirable path.)

Also, may I ask why this abstraction is necessary? It seems to me that the intent is to generalize the textinput stuff across HTMLInputElement and HTMLTextAreaElement, but I may be wrong on that.

Yes, that's the intent. The spec of the selection API is identical for both input and textarea, but until this change they both had separate implementations in the Servo codebase. However, the separate implementations were very similar and had clearly been copy/pasted. When I fixed the firing of the select event on assignment to selection{Start,End} for input, my fix didn't apply to textarea. To fix it for textarea would require me to make an identical change to HTMLTextAreaElement. It seemed more sensible to extract the common code so that the two elements behave the same way and there is no duplication. (This extraction implicitly fixes the implementation for textarea hence some more tests now pass.)

@KiChjang

This comment has been minimized.

Member

KiChjang commented Nov 18, 2017

A free function is a function that isn't inside an impl block, i.e. it exists at the top level of a module.

@jonleighton jonleighton force-pushed the jonleighton:issue-19171-2 branch from 94551bb to 65a4483 Nov 18, 2017

jonleighton added some commits Nov 17, 2017

Refactor selection-start-end test
Move assertions about the initial value of selection{Start,End} to their
own tests. This ensures that when one of these assertions fails, it
doesn't prevent other tests from being defined. Thus we have a clearer
view of which tests are passing or failing, since all tests get defined
regardless of which assertions fail.
Extract common text control selection code
The API for text control selection is the same for both <input> and
<textarea>:

https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#textFieldSelection

Before this change, they had similar but not identical implementations
with duplicate code. Now there is a common TextControl trait which
contains the implementation used by both. As a result, some previously
failing tests now pass.

@jonleighton jonleighton force-pushed the jonleighton:issue-19171-2 branch from 65a4483 to 6beda3c Nov 18, 2017

@jonleighton

This comment has been minimized.

Contributor

jonleighton commented Nov 18, 2017

Also ran ./mach update-manifest (what is the MANIFEST.json file for?)

@KiChjang

This comment has been minimized.

Member

KiChjang commented Nov 20, 2017

So I thought a bit about this, and was wondering whether the trait abstraction is necessary. It's true that HTMLInputElement and HTMLTextAreaElement don't share the selection code, but they both do have a textinput field in their structs. I'm wondering if the trait methods you define should better be located in textinput.rs.

@jonleighton

This comment has been minimized.

Contributor

jonleighton commented Nov 20, 2017

Yeah, I thought about that too. The reason I didn't go down that path is that TextInput wouldn't be able to handle anything DOM-related, i.e. firing events, marking the node dirty, etc. As the selection spec is further implemented, I expect there to be more common DOM-related code than there currently is, so it seems like a good idea to have an abstraction which can cover that stuff.

@KiChjang

This comment has been minimized.

Member

KiChjang commented Nov 20, 2017

I think it's actually doable to include DOM-related stuff as parameters to functions defined in textinput.rs, e.g. for set_selection_range, you can pass in a Window and Node parameter in order for it to performs its job.

@jdm

This comment has been minimized.

Member

jdm commented Nov 20, 2017

@bors-servo: r+
I am fine with the current abstraction. We can revisit the design as further changes are made.

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 20, 2017

📌 Commit 6beda3c has been approved by jdm

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 21, 2017

⌛️ Testing commit 6beda3c with merge 9797935...

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

Auto merge of #19272 - jonleighton:issue-19171-2, r=jdm
Further changes in relation to #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/19272)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 21, 2017

@bors-servo bors-servo merged commit 6beda3c into servo:master Nov 21, 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

jdm added a commit to web-platform-tests/wpt that referenced this pull request Jan 4, 2018

Refactor selection-start-end test
Move assertions about the initial value of selection{Start,End} to their
own tests. This ensures that when one of these assertions fails, it
doesn't prevent other tests from being defined. Thus we have a clearer
view of which tests are passing or failing, since all tests get defined
regardless of which assertions fail.

Upstreamed from servo/servo#19272 [ci skip]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment