Closed Bug 1792333 Opened 2 months ago Closed 2 months ago

Shift+Arrow on Searchfox behaves oddly.

Categories

(Core :: Layout, defect)

defect

Tracking

()

VERIFIED FIXED
107 Branch
Tracking Status
firefox-esr102 --- unaffected
firefox105 --- wontfix
firefox106 --- wontfix
firefox107 --- verified
firefox108 --- verified

People

(Reporter: emilio, Assigned: emilio)

References

(Blocks 2 open bugs, Regression)

Details

(Keywords: regression)

Attachments

(5 files)

STR:

ER:

  • Next line is selected (or so).

AR:

  • Most of the page is selected.

This seemed to work in 99 at least, running mozregression now.

Regressed by: 1475232
Flags: needinfo?(emilio)

Set release status flags based on info from the regressing bug 1475232

This shouldn't change behavior but makes the code easier to follow.

Assignee: nobody → emilio
Status: NEW → ASSIGNED

Instead of digging into the first line-iterable frame. Digging into the
first line-iterable frame is bogus, because if there are multiple flex
items we might prevent moving through them properly (see test-case).

The flex implementation is nice and fairly complete, IMO. The grid one
is not, but the resulting behavior is nicer than the actual one, is
sane, and matches Chrome in my testing.

In searchfox the behavior is even funnier because user-select: none is
involved, but that predates the regression.

Depends on D158085

Flags: needinfo?(emilio)

Testcase 2 is broken just like testcase 1 is right now.

I attached it since I suspect it might remain broken (though perhaps somewhat-less-broken) with the current version of the patch, since I think it violates the patch's current assumptions about "how many frames are on this line" and "what are the bounds of this line" as noted in this review comment:
https://phabricator.services.mozilla.com/D158086#inline-872164

Attachment #9296103 - Attachment description: Trivial test-case that doesn't work. → testcase 1: Trivial test-case that doesn't work.
Blocks: 1793251
Keywords: leave-open
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/66a061bd76fc
Various clean-ups to nsIFrame line-movement APIs. r=dholbert
Blocks: 1793322
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/502f1e7d287e
Factor out the "find closest frame in the line" checks. r=dholbert
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f4529232e0a8
Implement nsILineIterator in nsFlex/GridContainerFrame. r=dholbert
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/36212 for changes under testing/web-platform/tests
Regressions: 1793328
Keywords: leave-open
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/b85f9a5c6236
Account for windows style newlines in new tests.
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/e6150741c075
Actually deal with windows newlines.
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/33ce4b92678c
Deal with windows newlines in more places in the new tests.
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 107 Branch
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/ad67ead1905a
Obviously grid test needs the same tweak.
Blocks: 1793375
Upstream PR merged by moz-wptsync-bot
Regressions: 1794265
Flags: qe-verify+

Reproduced the issue mentioned in comment 0 using Firefox 107.0a1 (BuildId:20220924093346).

This issue is verified fixed using Firefox 108.0a1 (BuildId:20221020215126) and Firefox 107.0b3 (BuildId:20221020202724) on Windows 10 64bit, macOS 11 and Ubuntu 22.

During the verification of this issue, I've noticed that some issues still exist on testcase 2 (multi-line flexbox) but Bug 1793251 has been filled as a follow up.

It seems that we still have some differences between Chrome, Edge and Firefox on how browsers are behaving while traversing back up (Firefox doesn't automatically unselect the previously selected text) for the Searchfox case. Filled Bug 1796756 for more details.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.