Open Bug 1644742 Opened 4 years ago Updated 4 years ago

Triple-click selection broken on phabricator file names.

Categories

(Core :: DOM: Selection, defect, P3)

defect

Tracking

()

People

(Reporter: emilio, Unassigned)

Details

Attachments

(1 file)

STR:

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.

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.

STR:

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

Yeah, that sounds right. Sorry, I misdescribed :)

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

Hi Saschanaz,
Could you please help to take a look? I saw you recently working on triple-click. Thank you.

Severity: -- → S3
Flags: needinfo?(krosylight)
Priority: -- → P3

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

The chromium bug tracker may just be because of shadow DOM selection

Attached file bug1644742.html

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.

Flags: needinfo?(krosylight)
Flags: needinfo?(mbrodesser)

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.

Flags: needinfo?(mbrodesser)

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.

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.

Yes, I agree, may be worth just filing a Phabricator issue on this one.

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.

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.

Flags: needinfo?(mbrodesser)

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

Flags: needinfo?(mbrodesser)

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.

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.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: