584141 - Double-clicking doesn't select words across wbr elements

Status: VERIFIED FIXED

(Core :: DOM: Selection, defect)

Description

[Fredrik Roubert]

Attachment: 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.

Comment 2

[Alice0775 White]

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

So.... Shouldn't <wbr> act exactly like zero-width space wrt selection?  In other words, I think the current behavior is correct.

Comment 4

[Fredrik Roubert]

"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).

Comment 5

[Anthony Foiani]

(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;.

Comment 6

[(no longer active)]

I'll take this, but I'm swamped right now, so others should feel free to steal this from me if they want to!

Comment 7

[Javi Rueda :javirid]

Attachment: Longer testcase, as the original one is too short for the new minimum window size.

Comment 8

[(no longer active)]

(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?

Comment 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[(no longer active)]

(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?

Comment 11

[Sam]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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?

Comment 13

[Sam]

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 ­ wouldn't be better.

Comment 14

[Anthony Foiani]

(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."

Comment 15

[Reuben Morais [:reuben]]

I'm willing to work on this bug, do we have a decided behavior?

Comment 16

[Sam]

It doesn't look like we do.

Comment 17

[nickers]

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?

Comment 20

[liamlaverty]

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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

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?

Comment 22

[Anne (: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.

Comment 23

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

<wbr> doesn't seem semantically very different from ­ 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.

Comment 24

[Nour Saffour]

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 25

[Boris Zbarsky [:bzbarsky]]

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.

Comment 26

[Nour Saffour]

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.

Comment 27

[Nour Saffour]

(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?

Comment 28

[Boris Zbarsky [:bzbarsky]]

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.

Comment 29

[Nour Saffour]

(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?

Comment 30

[Nour Saffour]

Attachment: Bug 584141 Added WBR frame class implementing nsFrame and overriding thepeekOffset* methods.

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.

Comment 31

[Nour Saffour]

Attachment: Tests for wbr element Bug 584141

added tests for Bug 584141 selection across wbr elements

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/80ffb5ef8efb
Added WBR frame class implementing nsFrame and overriding thepeekOffset* methods. r=dbaron,Ehsan

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ab0ed4869416
Tests for wbr element Bug 584141 r=Ehsan

Comment 34

[Cosmin Sabou [:CosminS]]

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.

Comment 35

[Boris Zbarsky [:bzbarsky]]

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.

Comment 36

[(no longer active)]

(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.

Comment 37

[Arthur Iakab [arthur_iakab]]

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

Comment 38

[Nour Saffour]

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

Comment 39

[Tooru Fujisawa [:arai]]

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.

Comment 40

[Nour Saffour]

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

Going to find myself some other bug to work on.

Comment 41

[Kohei Yoshino [:kohei]]

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

Comment 42

[Fredrik Roubert]

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

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/613ca59a8e61
Tests for wbr element Bug 584141 r=Ehsan

Comment 44

[Stefan Hindli [:stefan_hindli]]

Backed out changeset 613ca59a8e61 (Bug 584141) for causing tier1 permafails in layout/generic/test/test_selection_doubleclick.html CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=5686e9cc48b26a8ec550f94091d5b8b4ed840f7d&selectedJob=272250403

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=272250403&repo=autoland&lineNumber=836

Comment 45

[Pulsebot]

Backout by [e-mail]:
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

Comment 46

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/15bc385e8692
Tests for wbr element Bug 584141 r=Ehsan

Comment 48

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/15bc385e8692

Comment 49

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15bc385e8692