Triple-click selection broken on phabricator file names.
Categories
(Core :: DOM: Selection, defect, P3)
Tracking
()
People
(Reporter: emilio, Unassigned)
Details
Attachments
(1 file)
508 bytes,
text/html
|
Details |
STR:
- Go to https://phabricator.services.mozilla.com/D79117
- Scroll down until you see the diff for
layout/printing/nsPrintJob.cpp
. - Triple-click to try to select the whole filename.
ER:
- The whole filename is selected.
AR:
- Only one word is selected.
This seems to work in Chromium, and I'm a bit confused because the markup seems really straight-forward, so I think something weirder is going on... Maybe some JS on the page or something?
At least fundamental Selection functionality doesn't seem to be broken, because triple-clicking
data:text/html,<span>x/y/z</span>
works.
Comment 2•4 years ago
|
||
Huh, that's really weird. Triple-click works fine to select the same path string (e.g. layout/printing/nsPrintJob.cpp
) where it appears earlier in the page (when mentioned in the various comments), but not where it appears as header for the diff.
Comment 3•4 years ago
|
||
STR:
- Go to https://phabricator.services.mozilla.com/D79117
- Scroll down until you see the diff for
layout/printing/nsPrintJob.cpp
.- Triple-click to try to select the whole filename.
AR:
- Only one word is selected.
Actually, what I see (on macOS, at least) is that if I triple-click, nothing is selected: after two clicks, one word is selected (as expected), but then the third click clears the selection (just as if there had been a long pause, so that it's a new single click rather than part of a multi-click action).
Reporter | ||
Comment 4•4 years ago
|
||
Yeah, that sounds right. Sorry, I misdescribed :)
Comment 5•4 years ago
|
||
Yeah.... if I set javascript.enabled
to false
and reload the phab page, there's no longer any problem triple-clicking those paths. (OTOH, the actual diffs don't load without JS, so it's not exactly useful, but does support the theory that some handler on the page is breaking this.)
Comment 6•4 years ago
|
||
Hi Saschanaz,
Could you please help to take a look? I saw you recently working on triple-click. Thank you.
Comment 7•4 years ago
|
||
Similar thing happens in Chromium bug tracker. Triple clicking in the third comment just cancels the selection. I'll take a look next week. https://bugs.chromium.org/p/chromium/issues/detail?id=1017039#c3
Reporter | ||
Comment 8•4 years ago
|
||
The chromium bug tracker may just be because of shadow DOM selection
Comment 9•4 years ago
|
||
The Phabricator case is because ev.target
behavior difference in selectstart
event. Both Gecko and Blink emit a text node as the target for a double click selection, but only Gecko emits the block parent for triple click selection. Phabricator somehow calls preventDefault()
when this happens.
Updated•4 years ago
|
Comment 10•4 years ago
|
||
Oops, submitted too early somehow.
Mirko, should Gecko follow the Blink behavior where a triple click selects the child nodes instead of the parent block? I think there is no spec to force any specific behavior.
(In reply to Emilio Cobos Álvarez (:emilio) from comment #8)
The chromium bug tracker may just be because of shadow DOM selection
Yes, that's at least one problem there.
(In reply to Kagami :saschanaz from comment #10)
Oops, submitted too early somehow.
Mirko, should Gecko follow the Blink behavior where a triple click selects the child nodes instead of the parent block? I think there is no spec to force any specific behavior.
Yes, unfortunately this isn't specified. I have no definite answer to the question. To achieve web-compatibility, it might be the right way, however, one would have to be careful to not break other Firefox-specific code.
Presumably, it could help to understand why Phabricator calls preventDefault
when the block parent is emitted. Depending how hard it's to debug, contacting a developer (don't know whom) from Phabricator could help.
Comment 13•4 years ago
|
||
Presumably, it could help to understand why Phabricator calls preventDefault when the block parent is emitted. Depending how hard it's to debug, contacting a developer (don't know whom) from Phabricator could help.
It's e.kill()
inside _onClickHeader
from differential.pkg.js
. (Fortunately it's not obfuscated, thanks Phabricator!). It listens mousedown
/selectstart
and calls e.getNode('changeset-header-path-name')
(which seems to check the ancestors of the event target, not sure), and if there is not such thing it calls e.kill()
which again calls e.preventDefault()
.
_onClickHeader: function (e) {
var
path_name = e.getNode('changeset-header-path-name');
if (path_name) {
return;
}
if (e.getType() === 'selectstart') {
e.kill();
return;
}
if (this._isSelected) {
this.getChangesetList().selectChangeset(null);
} else {
this.select(false);
}
},
I think the function is to mark the header as yellow when clicked, but not sure why it listens selectstart
then.
In order to determine how changing this would affect web-compatibility, it could be helpful to determine WebKit's behavior in this case. If it's the same like Chrome's it seems reasonable to follow.
Comment 15•4 years ago
•
|
||
Gnome Web behaves same as Chrome: Triple clicking the text node sets the target as the node, while triple clicking outer div (the right empty side of the div) sets the target as the parent. Visually they are same of course.
Not quite sure the difference is needed, IMO our behavior looks more consistent as it always selects the parent.
Reporter | ||
Comment 16•4 years ago
|
||
Yes, I agree, may be worth just filing a Phabricator issue on this one.
Comment 17•4 years ago
•
|
||
That said, both WebKit and Blink violate the spec where it requires not to fire selectstart with empty selection. Chrome does fire it whenever the div is clicked (including single clicks), and Phabricator uses that behavior to prevent selection while clicking the outer space of the header by listening and prevent-defaulting .selectstart
The user agent must not fire an event when the user agent sets the selection empty.
Edit: Chrome selects the invisible whitespace when double clicking, hence the target. And Phabricator uses this behavior. This is really weird and web pages should not depend on this.
Comment 18•4 years ago
|
||
That said again, the spec actually says nothing about a selection with a collapsed range (which is not logically "empty"). Chrome has confusing behavior here:
- Single click by mouse fires an event
- But it does not when moving the caret by keyboard inside a contenteditable event target. It fires an event only when non-collapsed selection occurs.
And the spec says nothing here. Mirko, is my understanding correct? Should the spec be revised to include "visually empty" selections?
I guess this specific behavior does not affect the Phabricator case, though.
(In reply to Kagami :saschanaz from comment #18)
That said again, the spec actually says nothing about a selection with a collapsed range (which is not logically "empty"). Chrome has confusing behavior here:
- Single click by mouse fires an event
- But it does not when moving the caret by keyboard inside a contenteditable event target. It fires an event only when non-collapsed selection occurs.
And the spec says nothing here. Mirko, is my understanding correct? Should the spec be revised to include "visually empty" selections?
No, presumably not, because selectstart
events should only be fired when "When the user agent is about to associate a new range newRange to the selection in response to a user initiated action".
Note the difference to selectionchange
events: "When the selection is dissociated with its range, associated with a new range or the associated range's boundary point is mutated".
I guess whether a user agent mutates an existing range or substitutes a range with another is not defined for clicking/typing.
I guess this specific behavior does not affect the Phabricator case, though.
Then let's deal with it separately. From your previous comment I get it's worth filing an issue for Phabricator.
Comment 20•4 years ago
•
|
||
I think I don’t fully understand comment #19. Currently it’s confusing to me that:
- Moving caret by keyboard does not dissociate-and-associate a range and so no selectstart
- Okay
- Creating visual selection by Shift key does dissociate-and-associate and so selectstart
- I guess it’s intuitively correct to get a selectstart, but logically why does it suddenly dissociate an existing one?
- Mouse clicks
- I think the behavior here should be same as moving caret by keyboard for consistency (no selectstart). Do you all agree? Or is there a reason to deviate?
(In reply to Kagami :saschanaz from comment #20)
I think I don’t fully understand Comment 19.
Which part?
Currently it’s confusing to me that:
- Moving caret by keyboard does not dissociate-and-associate a range and so no selectstart
- Okay
- Creating visual selection by Shift key does dissociate-and-associate and so selectstart
Is this also the case when extending a selection?
- I guess it’s intuitively correct to get a selectstart, but logically why does it suddenly dissociate an existing one?
I don't know.
- Mouse clicks
- I think the behavior here should be same as moving caret by keyboard for consistency (no selectstart). Do you all agree? Or is there a reason to deviate?
Intuitively, I agree. A reason to deviate would be if other browsers handle this differently.
Comment 22•4 years ago
|
||
Which part?
Basically, when to dissociate and when not to, which seems important to trigger selectstart. Is there a spec phrase that defines this?
Is this also the case when extending a selection?
Only when extending collapsed selection. I would understand this if the spec forced not to trigger selectstart for a collapsed selection (and defer until it un-collapses), but it does not AFAICT.
A reason to deviate would be if other browsers handle this differently.
Blink/WebKit do this differently.
(In reply to Kagami :saschanaz from comment #22)
Which part?
Basically, when to dissociate and when not to, which seems important to trigger selectstart. Is there a spec phrase that defines this?
That's undefined, AFAIK.
Is this also the case when extending a selection?
Only when extending collapsed selection. I would understand this if the spec forced not to trigger selectstart for a collapsed selection (and defer until it un-collapses), but it does not AFAICT.
A reason to deviate would be if other browsers handle this differently.
Blink/WebKit do this differently.
Comment 24•4 years ago
|
||
Filed https://discourse.phabricator-community.org/t/triple-clicking-file-path-in-a-diff-cancels-text-selection-only-on-firefox/4231. (So late, I know, sorry 😥)
Description
•