Closed Bug 584141 Opened 14 years ago Closed 5 years ago

Double-clicking doesn't select words across wbr elements

Categories

(Core :: DOM: Selection, defect)

Product:
Core
Component:
DOM: Selection
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla71
Tracking Flags:
Tracking Status
firefox71 --- fixed

People

(Reporter: roubert, Assigned: nsaffour, NeedInfo)

References

Details

(Keywords: html5, Whiteboard: [post-2.0][good first bug])

Attachments

(4 files)

A trivial test file.
123 bytes, text/html
Details
Longer testcase, as the original one is too short for the new minimum window size.
142 bytes, text/html
Details
Bug 584141 Added WBR frame class implementing nsFrame and overriding thepeekOffset* methods.
47 bytes, text/x-phabricator-request
Details | Review
Tests for wbr element Bug 584141
47 bytes, text/x-phabricator-request
Details | Review
Attached file A trivial test file. — 
The wbr element represents a line break opportunity:

http://www.w3.org/TR/html5/text-level-semantics.html#the-wbr-element

Currently, double-clicking on a word containing a wbr element won't select the entire word but only that part of the word on the side of the wbr element where the pointer is located.

This behaviour is unintuitive, has no support in the standards (that I can find) and differs from the behaviour of other browsers such as Chrome, Safari and Opera.
Component: General → Selection
Product: Firefox → Core
QA Contact: general → selection
Version: unspecified → Trunk
There are 2 regression window.
#1 regression window:
Works:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9a9pre) Gecko/2007110705 Firefox/3.0a9pre ID:2007110705
Fails (not expand selection if double click text after wbr):
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9b2pre) Gecko/2007110805 Firefox/3.0b2pre ID:2007110805
Suspected bug:
Bug 391584 - double-clicking a word followed by link selects the first word of the link text

#2 regression window:
Fails (not expand selection if double click text after wbr):
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1a1pre) Gecko/2008070803 Shiretoko/3.1a1pre ID:2008070803
Fails:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1a1pre) Gecko/2008070903 Shiretoko/3.1a1pre ID:2008070903
Suspected bug:
Bug 442554 - layout.word_select.eat_space_to_next_word should not change word selection method
So.... Shouldn't <wbr> act exactly like zero-width space wrt selection?  In other words, I think the current behavior is correct.
"The wbr element represents a line break opportunity."
(http://www.w3.org/TR/html5/text-level-semantics.html#the-wbr-element)

I can't see why this should cause only a part of the word to be selected (and neither could the authors of other major browsers, apparently).
(In reply to comment #3)
> So.... Shouldn't <wbr> act exactly like zero-width space wrt selection?  In
> other words, I think the current behavior is correct.

I ran across this bug on gmail, where they insert <wbr> elements into the subject when words get too long. In my case, these were SHA-1 hex hashes (so 40 characters), and gmail inserted the <wbr> after the first 30.

As more sites try to cater to small screens (especially on mobile devices), I suspect that <wbr> and &shy; will get more use.

I was going to say that <wbr> should just be &shy; without the hyphen, but apparently &shy; is a minefield:

  http://www.cs.tut.fi/~jkorpela/shy.html

If one uses the view that &shy; is a discretionary hyphen [rendering as hyphen+newline when used to break a word at end of line, invisible otherwise], then <wbr> seems to just be &shy; that doesn't render a hyphen.  Either way, I would expect word semantics (for selection, search, indexing, etc) to ignore <wbr> just as it should ignore &shy;.
I'll take this, but I'm swamped right now, so others should feel free to steal this from me if they want to!
Assignee: nobody → ehsan
Whiteboard: [post-2.0]
Whiteboard: [post-2.0] → [post-2.0][good first bug]
Whiteboard: [post-2.0][good first bug] → [post-2.0][good first bug][mentor=ehsan]
Attachment #541959 - Attachment mime type: text/plain → text/html
(In reply to comment #3)
> So.... Shouldn't <wbr> act exactly like zero-width space wrt selection?  In
> other words, I think the current behavior is correct.

Boris, do you still believe this is the case?
Last I checked, yes, because in all of the use cases people actually suggested for wbr on the mailing list it was in fact being used as a word separator in cases where words were run together for some reason.

But I don't feel particularly strongly about it.

The gmail thing just seems like abuse of wbr; it should simply use word-break; that's what that's for.
(In reply to comment #9)
> The gmail thing just seems like abuse of wbr; it should simply use word-break;
> that's what that's for.

Should we contact Google about this?
A use case occurs to me that disagrees with Boris but I might just not be understanding this correctly.  It seems like it might be reasonable to want to put a <wbr> tag after each slash in a long url (or other path) for example to make the path more legible.  In this case we would definitely still want a double-click to select the entire path.
We line-break after slashes in URLs anyway, without any <wbr> in sight.  So do other browsers.  So why would you want to do that?
Right, that makes sense.  I can't think of any other use where you would want the double-click to select the entire 'word' where a &shy; wouldn't be better.
(In reply to Boris Zbarsky (:bz) from comment #12)
> We line-break after slashes in URLs anyway, without any <wbr> in sight.  So
> do other browsers.  So why would you want to do that?

But what if there are cases that the browser doesn't know about, but the server does?

Some other possible use cases:

1. Breaking long identifiers between words (inspired by an example on http://www.w3schools.com/html5/tag_wbr.asp):

  "ThisIsMySuperLong<wbr>JavaMethodName"

2. Breaking genome strings on 3-tuple codons.

If you do want to require that <wbr> breaks words: is there any other portable way for a website author to indicate exactly where they want a long word broken, even using the CSS tricks you mention upstream?

A few other good links:
http://ejohn.org/blog/injecting-word-breaks-with-javascript/
http://dev.w3.org/html5/spec/text-level-semantics.html#the-wbr-element

Some (old) experiments with how "shy" and "wbr" interact with selection and clipboards:
http://www.w3.org/Bugs/Public/show_bug.cgi?id=9711

Anyway.  I would like to see it implemented so that wbr elements do not break words for selection / search purposes.  I will gladly admit that the actual use cases are pretty few, but it seems more in line with "do what the author would like" than "we're the browser, we know better."
I'm willing to work on this bug, do we have a decided behavior?
It doesn't look like we do.
It seems that Gmail puts <wbr/> into a 11-letter word in a mail subject after the 10th letter now, which makes the bug much more common.

Should W3C be requested a clarification on behavior of <wbr>? Has anyone contacted Google about the issue?
Keywords: html5
Mentor: ehsan
Whiteboard: [post-2.0][good first bug][mentor=ehsan] → [post-2.0][good first bug]
See Also: → 716399
See Also: → 1327803

I came across this today and assumed it was a bug. MSDN's technical documents use this html element to make sure their large class names appear nicely on mobile screens, similarly to the java examples above. However when you're on a desktop and double click to copy out the class name, you get a subset of the class name, which has consequences in this scenario, for example if I'm copying out an interface "IService" I end up copying out the implementation "Service" insead.

I think 8 and even 5 years ago this might have been an edge case with few obvious examples, but with the rise of long strings, especially in large text in we pages, #HashTagBugzillaIsCool for example, and the continued rise of use of mobiles internationally, more and more implementations of this html element will appear in the coming years, and Firefox's interpretation of the standard might be subverting the expectations of users and developers alike.

So if we want to change the behavior here, presumably we should have PeekOffsetWord just return CONTINUE for wbr? Seems like we could do that by just giving wbr a subclass of nsFrame that overrides that one method.

The question is whether this is a desirable change. Anne, David, thoughts?

Flags: needinfo?(dbaron)
Flags: needinfo?(annevk)

If that is what other implementations do that seems reasonable to me. It's unfortunate that selection isn't really maintained standards-wise, but maybe filing an issue against https://w3c.github.io/selection-api/ will resolve that at some point.

Flags: needinfo?(annevk)

<wbr> doesn't seem semantically very different from &shy; except that there isn't hyphenation introduced at the break point. So it seems like a reasonable change to me, particularly if it improves compatibility with other browsers. Modulo what Anne said in comment 22 about getting it standardized, of course.

Flags: needinfo?(dbaron)
Assignee: ehsan → nobody
Mentor: ehsan

Can I take this bug?
If yes, can someone point me out where to look?
This is my first bug on Bugzilla so a little help is appreciated :)

Comment 21 is probably a good place for the starting point, but I'm not 100% sure whether the claim in that comment is true; I am not an expert on the selection code, and comment 21 came after I spent a bit of time trying to read through it...

Unfortunately, it's possible that no one is really an expert on the selection code at this point. Which means this may or may not be good really first bug. ;) That said, if you're willing to deal with a bit of uncertainty, comment 21 is probably the place to start. What you would want to do is change the NS_NewWBRFrame to create an instance of the new nsFrame subclass that has the desired behavior.

A little uncertainty is always fun :)
I think I should be building a similar class to BRFrame.cpp.
https://dxr.mozilla.org/mozilla-central/source/layout/generic/BRFrame.cpp
Do you think this is correct?

Should I implement all the functions that the BRFRame implement? or only PeekOffsetWorld?

Please correct me if I'm going off track here.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #25)

Comment 21 is probably a good place for the starting point, but I'm not 100% sure whether the claim in that comment is true; I am not an expert on the selection code, and comment 21 came after I spent a bit of time trying to read through it...

Unfortunately, it's possible that no one is really an expert on the selection code at this point. Which means this may or may not be good really first bug. ;) That said, if you're willing to deal with a bit of uncertainty, comment 21 is probably the place to start. What you would want to do is change the NS_NewWBRFrame to create an instance of the new nsFrame subclass that has the desired behavior.

As far as I can see the 'NS_NewWBRFrame' is already using the 'nsFrame' subclass.
Is there a new version that it should use?

I think I should be building a similar class to BRFrame.cpp.

Sort of, yes. It doesn't have to be nearly as complicated.

Should I implement all the functions that the BRFRame implement? or only PeekOffsetWorld?

I would expect you only need PeekOffsetWord, but this is the part I am not really sure about. You might need some of the other PeekOffset* functions. You should definitely not need a custom Reflow or any of the size-getting complexity, since you want to leave the existing behavior there.

As far as I can see the 'NS_NewWBRFrame' is already using the 'nsFrame' subclass.

Right, but you want to create a new subclass of nsFrame, which will behave just like nsFrame except for the selection behavior. So you would only override the bits that have to do with selection, which I think are the PeekOffset* bits.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #28)

I think I should be building a similar class to BRFrame.cpp.

Sort of, yes. It doesn't have to be nearly as complicated.

Should I implement all the functions that the BRFRame implement? or only PeekOffsetWorld?

I would expect you only need PeekOffsetWord, but this is the part I am not really sure about. You might need some of the other PeekOffset* functions. You should definitely not need a custom Reflow or any of the size-getting complexity, since you want to leave the existing behavior there.

As far as I can see the 'NS_NewWBRFrame' is already using the 'nsFrame' subclass.

Right, but you want to create a new subclass of nsFrame, which will behave just like nsFrame except for the selection behavior. So you would only override the bits that have to do with selection, which I think are the PeekOffset* bits.

I think I can make it work from here.

Can I get the bug assigned to me please?

Assignee: nobody → nsaffour

The wbr element represents a line break opportunity. Before the change double clicking on a text that contains a wbr element did not select the whole sentence.
After the change, the presence of a wbr element in the selection does not affect the selection functionality.

Attachment #9097227 - Attachment description: Bug 584141 Added WBR frame class implementing nsFrame and overriding the peekOffsetWord method. → Bug 584141 Added WBR frame class implementing nsFrame and overriding thepeekOffsetWord method.
Attachment #9097227 - Attachment description: Bug 584141 Added WBR frame class implementing nsFrame and overriding thepeekOffsetWord method. → Bug 584141 Added WBR frame class implementing nsFrame and overriding thepeekOffset* methods.

added tests for Bug 584141 selection across wbr elements

Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/80ffb5ef8efb
Added WBR frame class implementing nsFrame and overriding thepeekOffset* methods. r=dbaron,Ehsan

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=271942929&resultStatus=testfailed%2Cbusted%2Cexception&revision=ab0ed4869416de1b5bc777d97949351c48b61913

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=271942929&repo=autoland&lineNumber=43507

Backout link: https://hg.mozilla.org/integration/autoland/rev/256c7cd198a50aaaeeae2889cdbbc5c8eab2789a

[task 2019-10-18T18:21:44.194Z] 18:21:44 INFO - TEST-PASS | layout/generic/test/test_selection_doubleclick.html | Select across WBR elements with double click: OK
[task 2019-10-18T18:21:44.195Z] 18:21:44 INFO - TEST-PASS | layout/generic/test/test_selection_doubleclick.html | Select across WBR elements using shift and right arrow: OK
[task 2019-10-18T18:21:44.195Z] 18:21:44 INFO - Buffered messages finished
[task 2019-10-18T18:21:44.199Z] 18:21:44 INFO - TEST-UNEXPECTED-FAIL | layout/generic/test/test_selection_doubleclick.html | Select across WBR elements using shift + ctrl + right arrow: OK - got "", expected "abcdef"
[task 2019-10-18T18:21:44.199Z] 18:21:44 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:322:16
[task 2019-10-18T18:21:44.199Z] 18:21:44 INFO - test@layout/generic/test/test_selection_doubleclick.html:53:7
[task 2019-10-18T18:21:44.200Z] 18:21:44 INFO - setTimeout handlerSimpleTest_setTimeoutShim@SimpleTest/SimpleTest.js:686:43
[task 2019-10-18T18:21:44.200Z] 18:21:44 INFO - window.onload@layout/generic/test/test_selection_doubleclick.html:74:42
[task 2019-10-18T18:21:44.200Z] 18:21:44 INFO - EventHandlerNonNull@layout/generic/test/test_selection_doubleclick.html:74:3
[task 2019-10-18T18:21:44.200Z] 18:21:44 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-10-18T18:21:44.200Z] 18:21:44 INFO - TEST-UNEXPECTED-FAIL | layout/generic/test/test_selection_doubleclick.html | Select across WBR elements using shift + ctrl + right arrow: OK - got "", expected "abcdef\nghijkl"
[task 2019-10-18T18:21:44.200Z] 18:21:44 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:322:16
[task 2019-10-18T18:21:44.200Z] 18:21:44 INFO - test@layout/generic/test/test_selection_doubleclick.html:63:7
[task 2019-10-18T18:21:44.200Z] 18:21:44 INFO - setTimeout handlerSimpleTest_setTimeoutShim@SimpleTest/SimpleTest.js:686:43
[task 2019-10-18T18:21:44.200Z] 18:21:44 INFO - window.onload@layout/generic/test/test_selection_doubleclick.html:74:42
[task 2019-10-18T18:21:44.200Z] 18:21:44 INFO - EventHandlerNonNull@layout/generic/test/test_selection_doubleclick.html:74:3
[task 2019-10-18T18:21:44.200Z] 18:21:44 INFO - TEST-PASS | layout/generic/test/test_selection_doubleclick.html | Select across WBR elements in paragraph with double click: OK

Please also use a commit message that looks smthg like: | Bug (no) - Add tests for wbr element. r=... | for eg.
Thank you.

Flags: needinfo?(nsaffour)

The Mac failure could be due to lines like this:

synthesizeKey("KEY_ArrowRight", {shiftKey:true, ctrlKey:true});

using ctrlKey. On Mac, it's the Option key, not the Ctrl key, that has the "extend the selection by a a word" behavior. If that is what's going on, it's possible that using "accelKey" instead of "ctrlKey" will do the right thing, or maybe it will send Command instead of Option. I can run the test on Mac if that would be useful to see what's going on there.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #35)

The Mac failure could be due to lines like this:

synthesizeKey("KEY_ArrowRight", {shiftKey:true, ctrlKey:true});

using ctrlKey. On Mac, it's the Option key, not the Ctrl key, that has the "extend the selection by a a word" behavior. If that is what's going on, it's possible that using "accelKey" instead of "ctrlKey" will do the right thing, or maybe it will send Command instead of Option. I can run the test on Mac if that would be useful to see what's going on there.

Yeah, I highly suspect that is the cause.

I ran try tests on Linux only hoping that would be sufficient, but I was wrong. Sorry for the hassle.

https://hg.mozilla.org/mozilla-central/rev/80ffb5ef8efb

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox71: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

I have a small question.

Why does it say that Jeff Walden is the one who wrote this file and it links to to the wrong bug?
https://hg.mozilla.org/mozilla-central/file/tip/layout/generic/WBRFrame.cpp

And its the same thing for many other files in layout/generic

Flags: needinfo?(nsaffour)

that field shows the details of "tip" revision (because the URL is for the revision),
and the file body is what you get when you checkout "tip" revision.

you can get the history of the specific file by clicking "revisions" link, or "annotate" link to get the info about which line is modified by who.

Thank you all for helping me with my first bug :)
Its been a pleasure!

Going to find myself some other bug to work on.

Verified with the latest Nightly build. In Bug 1477743, “SharedArrayBuffer” in the summary is now entirely selected with a double click. Thank you! 💯

Status: RESOLVED → VERIFIED

Verified with the latest Nightly build, very nice, thank you! 👍

Backout by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b0e81278d528
Backed out changeset 613ca59a8e61 for causing tier1 permafails in layout/generic/test/test_selection_doubleclick.html CLOSED TREE

needinfo because the tests got backed out and need to be fixed/relanded

Flags: needinfo?(ehsan)
Flags: needinfo?(ehsan)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: