Closed Bug 1738866 Opened 3 years ago Closed 1 year ago

New wpt failures in /html/semantics/forms/the-input-element/selection-pointer.html

Categories

(Core :: DOM: Core & HTML, defect, P2)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: wpt-sync, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [wpt])

Syncing wpt PR 31451 found new untriaged test failures in CI

Tests Affected

Firefox-only failures

/html/semantics/forms/the-input-element/selection-pointer.html
Selecting texts across <input type=button> should not cancel selection: FAIL
Selecting texts across <input type=color> should not cancel selection: FAIL
Selecting texts across <input type=range> should not cancel selection: FAIL
Selecting texts across <input type=reset> should not cancel selection: FAIL
Selecting texts across <input type=submit> should not cancel selection: FAIL

CI Results

Gecko CI (Treeherder)
GitHub PR Head

Notes

These updates will be on mozilla-central once bug 1738785 lands.

Note: this bug is for tracking fixing the issues and is not
owned by the wpt sync bot.

This bug is linked to the relevant tests by an annotation in
https://github.com/web-platform-tests/wpt-metadata. These annotations
can be edited using the wpt interop dashboard
https://jgraham.github.io/wptdash/

If this bug is split into multiple bugs, please also update the
annotations, otherwise we are unable to track which wpt issues are
already triaged. Resolving as duplicate or closing this issue should
be cause the bot to automatically update or remove the annotation.

Severity: -- → S3
Priority: -- → P2

Hi Masayuki, can you please take this?

Flags: needinfo?(masayuki)

Hmm... I'm not sure the test is checking reasonable result. It ends the drag of extending selection at {x: 0, y: 0} at the third line (of course, this is an edge case from the user operation point of view). According to the failure log, our event handler in layout does not extend the selection to start of text node, instead, stops extending the selection at the <br> in the second line. If changing the behavior does not have any side-effects, we maybe can change our behavior. But otherwise, just changing the tests is more reasonable...

Flags: needinfo?(masayuki)

Oh, our selection handling is also buggy. In the failure cases, Selection.focusNode and Selection.anchorNode (and corresponding offsets) are not mach with Selection.getRangeAt(0)'s start/end containers.

If I change this line:
https://searchfox.org/mozilla-central/rev/f9beb753a84aa297713d1565dcd0c5e3c66e4174/testing/web-platform/tests/html/semantics/forms/the-input-element/selection-pointer.html#40

assert_equals(selection.anchorNode, foo.childNodes[0], "anchorNode");

to

assert_equals(selection.getRangeAt(0).startContainer, foo.childNodes[0], "anchorNode");

, the result becomes:

  UNEXPECTED-PASS Selecting texts across <input type=button> should not cancel selection
  UNEXPECTED-PASS Selecting texts across <input type=color> should not cancel selection
  UNEXPECTED-PASS Selecting texts across <input type=range> should not cancel selection
  UNEXPECTED-PASS Selecting texts across <input type=reset> should not cancel selection
  UNEXPECTED-PASS Selecting texts across <input type=submit> should not cancel selection

Then, revert it and change the next line from:

assert_equals(selection.focusNode, bar.childNodes[0], "focusNode");

to

assert_equals(selection.getRangeAt(0).endContainer, bar.childNodes[0], "focusNode");

, the result is same as current expectation. Therefore, we fail only the anchor boundary's management.

Ah, I see. When this happens, the rangeCount becomes 2. I.e., 2 selection ranges are created before and after the <input> element.

It seems that fixing this requires stop our traditional behavior that is splitting selection ranges at non-seletable nodes (bug 1773065). Of course, it's risky to change. However, this incompatible behavior causes the incompatible behavior of Selection.focusNode, Selection.focusOffset, Selection.anchorNode, Selection.anchorOffset and Selection.rangeCount even without users' intentional multiple ranges selection. Ideally, we should fix this, but I worry about the risk. What do you think (see also bug 1773065 comment 5)?

Flags: needinfo?(htsai)

I am a bit curious then why the test fails on certain input types but not all. Do we handle selection differently for different input types?

Flags: needinfo?(htsai)

(I want to keep the NI)

Flags: needinfo?(htsai)

To answer comment 6, I'm uncomfortable that we rush into fixing this risky interop issue without seeing more benefits for webcompat. That is, I can't find severe or outstanding web breakages due to this, so I am concerned about the payoff of taking the risk to change our traditional behavior.

:zcorpan filed https://github.com/web-platform-tests/interop/issues/313

Flags: needinfo?(htsai)

(In reply to Hsin-Yi Tsai (she/her) [:hsinyi] from comment #7)

I am a bit curious then why the test fails on certain input types but not all. Do we handle selection differently for different input types?

Only some types of <input> has anonymous nodes which is not selectable, e.g., labal in the button. If there is no such nodes like <input type="text">, selection range across the <input> won't be split.

(In reply to Hsin-Yi Tsai (she/her) [:hsinyi] from comment #9)

To answer comment 6, I'm uncomfortable that we rush into fixing this risky interop issue without seeing more benefits for webcompat.

Sure.

That is, I can't find severe or outstanding web breakages due to this, so I am concerned about the payoff of taking the risk to change our traditional behavior.

We could meet the breakage since it looks like that Selection lies if web apps access the information only with focusNode, focusOffset, anchorNode and anchorOffset, and fixing this is too risky. Therefore, I wonder, it might be better to prepare for the worst scenario in some spare times only in the Nightly channel.

With the wpts updated in https://github.com/web-platform-tests/wpt/pull/39753 , all pass on Firefox currently.

Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.