Text Selection API for Input and TextArea #804

sjelin · 2014-06-24T01:17:39Z

See #799. Followed documentation at http://www.whatwg.org/specs/web-apps/current-work/#textFieldSelection

Pretty straight forward addition. A few concerns:

I ended up duplicating the code in order to put it into both Input and Text Area
When the content of the text box changes, presumably the selection is also supposed to change. Consider, for example, if the content of the box is empties, and the selection is supposed to be of characters 4 through 10. Clearly that selection is now impossible. However, I couldn't really find any documentation on this point (though it's possible I don't know where to look). Chrome moves the cursor to the end of the text box, which seems reasonable, but since I couldn't find it actually documented anywhere I just did some basic sanitizing to make sure the selection is always within the bounds of the actual text.
Sometimes input tags don't support the selection API (e.g. type=radio). In these cases I throw throw new core.DOMException(core.INVALID_STATE_ERR), because chrome throws an InvalidStateError and this was the most similar thing I could find. No idea if that's the correct thing to throw though. Would appreciate someone who knows more taking a look. Never mind, the W3C tests seem to imply that this is the correct thing to throw.
On my machine, the automated tests stall at running jsdom/index.js jsdom/index. They did this before I made my changes, so I'm pretty sure it's not my fault, but at the same time I feel kind of queasy sending a pull request without having the tests finish. This stopped happening when I rebased against 1.0 (yay?)
~~On that note, I haven't written any tests. I did some manual testing, so I know it everything works. I don't know if that's enough for you guys.~~ See the discussion below

Also, the last three commits are all just small bug fixes. The "merge" was just that I stupidly tried to amend my first bug fixing commit after pushing it to github.

sjelin · 2014-06-30T16:23:30Z

@domenic What's going on with this? You need me to write tests or something? Or is there a more fundamental problem?

domenic · 2014-06-30T16:29:10Z

I hadn't had time to review such a large contribution. But yes, doing a quick glance-through, there definitely need to be tests. Please read the CONTRIBUTING.md?

sjelin · 2014-06-30T16:31:23Z

Alright, will do

sjelin · 2014-06-30T23:54:10Z

Alright, I read CONTRIBUTING.md, checked to see if there were any existing tests which had been disabled (there weren't), and found the relevant tests in the w3c/web-platform-tests project (they're here and here, in case anyone is interested). The CONTRIBUTING.md wasn't totally clear, so I hope you don't mind if I ask some obvious questions:

Where exactly do I put the tests? My guess is in test/level2/html.js with the other tests for the same elements (e.g. HTMLInputElement23), but I'd like some confirmation.
The tests don't match the spec. Specifically, this test seems to think that <input type="email"> should support the selection API, but if you look at the spec it very clearly says that <input type="email"> should not. Presumably I fix W3C's test when I port it over, right? The only concern I have is that if you start running W3C's tests directly you'll get an error.

domenic · 2014-06-30T23:56:45Z

OK, great, happy to help.

It's interesting that you've found a spec and/or test bug. Which do browsers implement? That determines whether we should fix the tests or the spec.

To get the tests to run, the easiest thing to do is to retarget the PR against the 1.0 branch, and then just point at the URLs by adding them to test/w3c/index.js, like so. We are pretty close to merging 1.0 anyway.

sjelin · 2014-07-01T00:22:52Z

Chrome matches the spec, not the tests. Firefox and Safari matches the tests, not the spec (though their implementation of the API is riddled with problems). If I'm going to just point to the URL for the W3C test, doesn't that mean I kind of have to match up with the tests, rather than the spec? I could just submit a pull request to W3C's test of course (it's an extremely quick fix), but that might take some time to go through and we probably don't want the tests failing in the mean time.

Also, I recently rebased against master. Will that be a problem w.r.t. merging into 1.0? Should I just rebase against 1.0? (Can you tell I'm bad at git?)

domenic · 2014-07-01T00:27:02Z

If I'm going to just point to the URL for the W3C test, doesn't that mean I kind of have to match up with the tests, rather than the spec? I could just submit a pull request to W3C's test of course (it's an extremely quick fix), but that might take some time to go through and we probably don't want the tests failing in the mean time.

Yeah, I was going to say, we could do a pull request on the tests if they are wrong. And great, now we don't know who is wrong. Will file a bug against the spec and see what happens. In the meantime matching the tests is probably the way to go, I think? Unless it's much more work.

Also, I recently rebased against master. Will that be a problem w.r.t. merging into 1.0? Should I just rebase against 1.0? (Can you tell I'm bad at git?)

Rebasing against 1.0 is great. You may need to either edit this PR to target 1.0, or close it and open a new one, through the GitHub UI. (I don't know if the edit option is possible.) It doesn't matter too much since I can always do the rebasing myself locally even if GitHub gets confused and thinks you're targeting master.

And hey, if you know what "rebasing against 1.0" is, you're better at Git than a lot of people on GitHub, so yay :)

sjelin · 2014-07-01T01:13:11Z

It's not really any work to match the tests. Just have to add an extra case to a conditional.

I literally learned what rebasing was today, specifically to keep this pull request up to date =P

sjelin · 2014-07-01T05:20:12Z

Hey, I did what you said, but I'm not really sure the W3C tests are actually running. Like, there's no console output along the lines of running w3c/index.js or anything. I get the following:

running level1/core.js level1/core
running level1/html.js level1/html
running level1/svg.js level1/svg
running level2/core.js level2/core
running level2/html.js level2/html
running level2/style.js level2/style
running level2/extra.js level2/extra
running level2/events.js level2/events
running level3/textContent.js level3/textContent
running level3/xpath.js level3/xpath
running living-dom/attributes.js living-dom/attributes
running living-dom/compare-document-position.js living-dom/compare-document-position
running living-dom/node-contains.js living-dom/node-contains
running living-dom/node-parent-element.js living-dom/node-parent-element
running living-html/navigator.js living-html/navigator
running window/index.js window/index
running window/history.js window/history
running window/script.js window/script

I checked test/runner and it does have w3c/index.js listed so I'm not sure what's up. I also ran npm test -s w3c/index.js, which also produced no output. Then, because I was concerned, I changed something to make a test fail, reran npm test -s w3c/index.js, and still got no output. Does this feature just not work yet?

sjelin · 2014-07-02T02:44:13Z

Update: The tests are actually inconsistent with each other too. This test thinks <input type="email"> shouldn't support the selection API, and the one I linked to before thinks it should. Also, both tests test for exactly the same thing. So I think the best plan would be to just ignore the test which doesn't conform to the spec.

domenic · 2014-07-02T02:48:50Z

Oh, wow, that is awesome.

Regarding problems running the tests: ugh. I bet this is an instance of our recurring "some tests crash the runner so hard it exits with no output" bug.

What I normally do in those situations is try to run the last line independently, with -v. So

node test/runner -s window/script -v

and see what happens.

Sebmaster · 2014-07-07T17:56:29Z

I bet this is an instance of our recurring "some tests crash the runner so hard it exits with no output" bug.

As far as I understood it from the times it happened to me it seems like this happens when a test is async and the test.done() method doesn't get called because some event doesn't fire correctly.

@sjelin Since w3c-test.org is currently down you can rebase against #824 which allows local testing and doesn't rely on the website (for simple tests).

sjelin · 2014-07-09T09:24:27Z

Hey, I've gotten super busy but I'll deal with this next week ok?

domenic · 2014-07-09T15:47:34Z

No problem!!

fluffybunnies · 2015-01-19T07:42:57Z

lib/jsdom/level2/html.js

this._selectionStart ?

windyarts · 2015-03-13T04:32:24Z

Will you merge this?

domenic · 2015-03-13T04:33:43Z

It's still missing tests :(

sjelin · 2015-03-13T06:38:00Z

Wow, totally forgot about it. I'll deal with it this weekend, sorry

On Thu, Mar 12, 2015 at 9:33 PM, Domenic Denicola notifications@github.com
wrote:

It's still missing tests :(

—
Reply to this email directly or view it on GitHub
#804 (comment).

benmosher · 2015-03-30T14:21:43Z

Looking forward to this! 👍

kentmw · 2015-04-15T21:13:02Z

Also looking forward to this. :)

Returns an empty object. If you need it to act more realistically, the spec states a Selection object should be returned. See https://developer.mozilla.org/en-US/docs/Web/API/Selection and http://www.quirksmode.org/dom/range_intro.html This should arguably be implemented in jsdom rather than zombie - some work has been done to implement selections in Inputs and TextAreas (jsdom/jsdom#804) but not merged at time of writing.

quantizor · 2015-11-26T00:57:05Z

@sjelin Looks like this needs a rebase, when you get the chance.

domenic · 2015-11-26T00:58:37Z

I can do the rebase if someone writes tests

quantizor · 2015-11-26T01:09:25Z

@domenic I'll take a crack at this - want me to submit as a new PR since I can't add onto this one? Cherry picked with his authorship of course

domenic · 2015-11-26T01:17:02Z

That sounds great! Just be sure to read CONTRIBUTING.md about how to turn on web platform tests that exist already, and how new tests should be written in that style. Feel free to bug us in #jsdom on freenode IRC if you have any questions.

sjelin · 2015-11-26T05:46:06Z

It's a thanksgiving miracle! Also, sorry guys, I got very sick shortly after sending this PR and then switched jobs so it was very low priority. @yanovich let me know if you have any questions. I also give you permission to take full authorship if that makes your life easier.

domenic · 2015-12-06T20:26:10Z

Superceded by #1305 :)

sjelin added 5 commits June 23, 2014 19:55

added text selection API for input tag

242a282

added text selection API for textarea

57b5d8c

forgot to test, there were some bad typos in my last two commits

ac2b6d4

forgot to test, there were some bad typos in my last two commits

ec7b4fb

Merge branch 'master' of https://github.com/sjelin/jsdom

e7cc5b4

This was referenced Jun 24, 2014

Text Selection API for Input and TextArea #803

Closed

input.selectionStart is undefined #799

Closed

fluffybunnies reviewed Jan 19, 2015
View reviewed changes

lib/jsdom/level2/html.js

Copy link

Contributor

fluffybunnies Jan 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this._selectionStart ?

Sebmaster mentioned this pull request Mar 20, 2015

Support for WebComponents API #1030

Closed

domenic force-pushed the master branch from 555c785 to ffa4039 Compare September 14, 2015 14:37

domenic added the needs tests label Nov 26, 2015

domenic force-pushed the master branch from 42f7a85 to c6e1524 Compare November 26, 2015 15:37

quantizor mentioned this pull request Nov 30, 2015

Input + Textarea selection API #1305

Closed

domenic closed this Dec 6, 2015

Uh oh!

Text Selection API for Input and TextArea #804

Text Selection API for Input and TextArea #804

Uh oh!

Conversation

sjelin commented Jun 24, 2014

Uh oh!

sjelin commented Jun 30, 2014

Uh oh!

domenic commented Jun 30, 2014

Uh oh!

sjelin commented Jun 30, 2014

Uh oh!

sjelin commented Jun 30, 2014

Uh oh!

domenic commented Jun 30, 2014

Uh oh!

sjelin commented Jul 1, 2014

Uh oh!

domenic commented Jul 1, 2014

Uh oh!

sjelin commented Jul 1, 2014

Uh oh!

sjelin commented Jul 1, 2014

Uh oh!

sjelin commented Jul 2, 2014

Uh oh!

domenic commented Jul 2, 2014

Uh oh!

Sebmaster commented Jul 7, 2014

Uh oh!

sjelin commented Jul 9, 2014

Uh oh!

domenic commented Jul 9, 2014

Uh oh!

fluffybunnies Jan 19, 2015

Choose a reason for hiding this comment

Uh oh!

windyarts commented Mar 13, 2015

Uh oh!

domenic commented Mar 13, 2015

Uh oh!

sjelin commented Mar 13, 2015

Uh oh!

benmosher commented Mar 30, 2015

Uh oh!

kentmw commented Apr 15, 2015

Uh oh!

quantizor commented Nov 26, 2015

Uh oh!

domenic commented Nov 26, 2015

Uh oh!

quantizor commented Nov 26, 2015

Uh oh!

domenic commented Nov 26, 2015

Uh oh!

sjelin commented Nov 26, 2015

Uh oh!

domenic commented Dec 6, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants