Closed Bug 1774697 Opened 2 years ago Closed 2 years ago

Ignore the computed "user-select" on editable elements (i.e. use the default behavior even if "none" was specified)

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
106 Branch
Tracking Flags:
Tracking Status
firefox106 --- fixed

People

(Reporter: dholbert, Assigned: tlouw)

References

(
URL
)

Details

Attachments

(3 files)

testcase 1
1.17 KB, text/html
Details
Bug 1774697 - Ignore the computed "user-select" on editable elements r=dholbert
48 bytes, text/x-phabricator-request
Details | Review
testcase 2: using "all" instead of "none"
1.18 KB, text/html
Details

In bug 1773810 (and formerly in https://github.com/webcompat/web-bugs/issues/22784#issuecomment-447699561 ) , we noticed that Firefox has broken-feeling selection behavior in textareas if user-select:none is active, while Chrome does not.

It turns out Chrome's behavior seems to match the spec. (Or perhaps the spec was written based on Chrome's behavior? In any case, I think it makes sense to match, probably.)

https://drafts.csswg.org/css-ui/#content-selection
Specific relevant text spec-here:

The used value is the same as the computed value, except:

  1. on editable elements where the used value is always contain regardless of the computed value

We don't literally support the contain value for this property, but that value just describes the-behavior-that-auto-has-on-editable-elements:

The used value of auto is determined as follows:
[...]
If the element is an editable element, the used value is contain

So essentially: from a used-value perspective, we should behave as if all editable elements had their default user-select:auto styling.

This would fix bug 1773810.

Attached file testcase 1 —
Summary: Ignore the computed "user-select" on editable elements → Ignore the computed "user-select" on editable elements (i.e. use the default behavior even if "none" was specified)

Chrome gives "expected results" on the attached testcase; they allow all of the editable fields to be selectable and editable-as-normal. I think this matches the spec, per comment 0.

Firefox gives "actual results", not allowing any of the editable fields to be selected using the mouse.
Safari gives the same results as Firefox, plus they also don't allow the editable fields to be edited.

Tentatively assigning to dshin per slack discussion, since he's already done the hard work of identifying the problem in bug 1773810. I think this should be relatively straightforward to fix via searching for userselect and seeing which of those callsites are relevant for editable elements in the testcase here.

(I'm not sure offhand what the appropriate check to use would be for "is this thing editable", but hopefully we already have an API that would be appropriate to tell us that.)

Assignee: nobody → dshin

(In reply to Daniel Holbert [:dholbert] from comment #4)

Tentatively assigning to dshin per slack discussion

(update: after talking with David and Emilio, let's point Tiaan at this. Tiaan: see comment 0 and comment 5, and emilio & I can both provide more context if helpful. :))

Assignee: dshin → tlouw

How would I automate a test for the correct behavior?

The new behavior should cause the existing layout/base/tests/bug1518339-2.html to fail, presumably. You can run it with layout/base/tests/test_reftests_with_caret.html (probably using this to debug).

So that test needs to be removed probably, and then something like this (untested, but should be close enough) could work as a WPT test (probably around testing/web-platform/tests/editing or testing/web-platform/tests/selection or so...):

<!doctype html>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<script src="/resources/testdriver.js"></script>
<script src="/resources/testdriver-actions.js"></script>
<script src="/resources/testdriver-vendor.js"></script>
<style>
  textarea, input, div { user-select: none }
</style>
<textarea rows=1>I should be selectable</textarea>
<input type=text value="I should be selectable">
<div contenteditable="true">I should be selectable</div>
<script>
promise_test(async function() {
  for (let element of document.querySelectorAll("textarea,input") {
    element.focus();
    let start = element.selectionStart;
    await test_driver.click(element);
    assert_not_equals(element.selectionStart, start, "Selection should've moved on click");
  }
  let div = document.querySelector("div");
  div.focus();
  let oldOffset = getSelection().focusOffset;
  await test_driver.click(div);
  assert_not_equals(oldOffset, getSelection().focusOffset, "Selection should've moved on click");
});
</script>

Check for a text input or editable host first, because they always set
the used style to 'text' (as is described in the spec)

https://drafts.csswg.org/css-ui-4/#content-selection

(In reply to Emilio Cobos Álvarez (:emilio) from comment #8)

The new behavior should cause the existing layout/base/tests/bug1518339-2.html to fail, presumably.

It doesn't, actually -- that test still passes with the attached patch, because user-select: all still differs from the default behavior on editable elements.

Here's a testcase to exercise/demonstrate this (like the first testcase but with all instead of none).

I'm pretty sure it's a bug for all to behave specially, per the spec text quoted in comment 0, so we need to fix this as well (possibly in a "part 2" patch, or in a followup bug if it's complex for some reason).

The expected behavior for testcase 2 is that all of the editable elements should allow individual character selection as-usual, as if user-select weren't specified.

Chromium matches this expectation. Gecko (with or without the current patch) seems to reject mouse-selections entirely in the textfield-like areas, and only allows selection-in-total in the contentediatble div. WebKit allows selection-in-total of some of the textfield-like areas and rejects selections in others.)

In the UsedUserSelect function there is a check for aFrame->IsTextInputFrame() || IsEditingHost(aFrame) to see if it is selectable. A frame with user-select: all fails this check. After investigating I found the following:

https://searchfox.org/mozilla-central/source/layout/generic/nsIFrame.cpp#5667

By removing the check for StyleUserSelect::All, it looks like it is using one of the parent frames and then the checks in UsedUserSelect succeeds. My assumption is that an element with user-select: all creates a child element that is used for representing the atomic selection, but I do not have enough knowledge right now to verify this. I did the following try which doesn't seem to reveal any issues with that check removed:

https://treeherder.mozilla.org/push-health/push?repo=try&revision=37017fd70c9972e2f380d6c426c04da8b9904238&tab=tests&testGroup=ki

(In reply to Tiaan Louw from comment #12)

After investigating I found the following:

https://searchfox.org/mozilla-central/source/layout/generic/nsIFrame.cpp#5667

(Here's a perma-link, from hotkey y on Searchfox:
https://searchfox.org/mozilla-central/rev/c30349265c87047a324a471d2f39a216b6749262/layout/generic/nsIFrame.cpp#5667
)

Digging through blame, it looks like that check (for userSelect == StyleUserSelect::All || frame->IsGeneratedContentFrame()) goes back ages; it was added here in 2005 for bug 316281:
https://searchfox.org/mozilla-central/diff/a710fcd15d380fdeebbc3f90abdaae355cefd617/layout/generic/nsFrame.cpp#2085
...under a different function-name, and then it was split out into the function's current name in bug 329031:
https://searchfox.org/mozilla-central/diff/96bf85b6acc8c798b2d50bf876630de822ae75de/layout/generic/nsFrame.cpp#2611

I think this all stuff needs further investigation and might (?) have some fiddly special-cases / side-effects...

Right now, I'd probably recommend spinning that part off to a followup lower-priority bug, and not blocking this fix on it; though, as noted in phab, we should broaden the test so that it exercises all (and we can annotate that part of the test as expected-fail).

I added all the user-select values to the test and marked the all values as expected fail, which they do. Latest changes in phabricator.

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1861b83150d9
Ignore the computed "user-select" on editable elements r=dholbert

(In reply to Narcis Beleuzu [:NarcisB] from comment #16)

Backed out for wpt failure on user-select-on-input-and-contenteditable.html
Log link: https://treeherder.mozilla.org/logviewer?job_id=383966196&repo=autoland&lineNumber=6934

This is stuff like:

INFO - TEST-UNEXPECTED-FAIL | /selection/user-select-on-input-and-contenteditable.html | selection for text - promise_test: Unhandled rejection with value: object "Error: element click intercepted error"

Please also take a look over https://treeherder.mozilla.org/logviewer?job_id=383969198&repo=autoland&lineNumber=13958

This one seems to be just a reduction in assertion-count; the testcase has asserts(16-26) but we only asserted 11 times.

This is for crashtest 1644819.html that happens to have a contenteditable element with -webkit-user-select: none. Probably we just take a different code-path when rendering that testcase now (due to this bug's commit) and we bypass some of the assertions that we used to hit, or maybe hit them fewer times.

hg-blame-history seems to show that we've broadened the allowed-assertion-count for this test over time; I'm not sure how much of that is due to randomness (i.e. if we only actually reach 26 assertions some of the time) vs. platform differences (if we reliably hit 26 assertions on some platforms).

In a perfect world, we'd take this opportunity to find the new low/high values for the assertion range here via some try pushes where you remove the asserts annotation entirely to gather some data.

But if you like, it's probably also fine to just reduce the lower-bound to 11 and call it good (assuming that's really the new lower bound).

I will be taking a look at this.

Flags: needinfo?(tlouw)

I'm running tests to figure out the min and max values for assertion count in the crashtests.

For the wpt test failure, it looks like it might be a flaky test, but will run repeater verify tests to confirm.

I will set the assertion count according to the tests:

linux = 11
osx = 13
Windows 10 = 12
Android = 11

(In reply to Tiaan Louw from comment #20)

I will set the assertion count according to the tests:

Sounds like you're talking about a single specific count per platform (rather than a range)? If so: that's great!

To be on the safe side, it'd probably be worth pushing the patch with your proposed assertion-counts to Try (including "debug crashtest" in your fuzzy-filter, at least; assertions are disabled in opt builds, so you could exclude them if you don't need to Try them for other investigations). And then, you can do a handful of retriggers on the crashtest tasks and take note of any oranges for occasional higher/lower values, and update your counts to be ranges if necessary.

Hopefully your counts are spot on; but given that this test had an asserts(N-M) range up until this point, it seems possible that it still requires a range like that. (Though it's possible the range was required due to some nondeterminism related to the user-select behavior that you're changing here; and/or due to folks simply updating the assertion-count by broadening the range when really it was just a fixed count that changed with a particular commit.)

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:tlouw, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Flags: needinfo?(tlouw)
Flags: needinfo?(dholbert)
Flags: needinfo?(dholbert)

Fixing tests

Flags: needinfo?(tlouw)

The issue with the tests were that the clicks were not processed and the tests failed. After investigating i added a <br> to the end of each input to make sure they are each on a separate line. That solved the tests and all the clicks were working. (There were still failing tests, but they were marked expected to fail). The suggestion was to remove the <br> tags again, mark those tests expected to fail as well and report the issue as another bug. Removing the tags causes a completely different error which I cannot explain. They look like server errors?? This is the log I keep getting after 3 retries:

https://treeherder.mozilla.org/logviewer?job_id=388338897&repo=try&lineNumber=1773

I'm not sure what the issue is with the tests if the <br> tags are not there.

Flags: needinfo?(dholbert)

That's complaining about a syntax error on your test annotation: https://treeherder.mozilla.org/logviewer?job_id=388338897&repo=try&lineNumber=1816:

Strings starting 'if ' must be quoted (expressions must start on a newline and be indented): /builds/worker/workspace/build/tests/web-platform/meta/selection/user-select-on-input-and-contenteditable.html.ini line 7

Right you are, need to read the fine print. Thanks

Flags: needinfo?(dholbert)
Attachment #9282001 - Attachment description: Bug 1774697 - Ignore the computed "user-select" on editable elements r=dholbert → WIP: Bug 1774697 - Ignore the computed "user-select" on editable elements r=dholbert
Attachment #9282001 - Attachment description: WIP: Bug 1774697 - Ignore the computed "user-select" on editable elements r=dholbert → Bug 1774697 - Ignore the computed "user-select" on editable elements r=dholbert
Pushed by tlouw@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/daf4a3797e50
Ignore the computed "user-select" on editable elements r=dholbert
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/35816 for changes under testing/web-platform/tests

https://hg.mozilla.org/mozilla-central/rev/daf4a3797e50

Status: NEW → RESOLVED
Closed: 2 years ago
status-firefox106: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 106 Branch
Upstream PR merged by moz-wptsync-bot
See Also: → 1796813
Duplicate of this bug: 684829
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: