1792333 - Shift+Arrow on Searchfox behaves oddly.

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

STR:

• Select some text in https://searchfox.org/mozilla-central/source/browser/base/content/test/general/browser_newwindow_focus.js#76
• Press Shift + Arrow down.

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.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: testcase 1: Trivial test-case that doesn't work.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1792333 - Various clean-ups to nsIFrame line-movement APIs. r=#layout

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

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1792333 - Implement nsILineIterator in nsFlex/GridContainerFrame. r=dholbert,#layout

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

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (multi-line flexbox)

Comment 6

[Daniel Holbert [:dholbert]]

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1792333 - Factor out the "find closest frame in the line" checks. r=dholbert

Depends on D158085

Depends on D158085

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/66a061bd76fc
Various clean-ups to nsIFrame line-movement APIs. r=dholbert

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/502f1e7d287e
Factor out the "find closest frame in the line" checks. r=dholbert

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f4529232e0a8
Implement nsILineIterator in nsFlex/GridContainerFrame. r=dholbert

Comment 11

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/36212 for changes under testing/web-platform/tests

Comment 12

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/66a061bd76fc

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b85f9a5c6236
Account for windows style newlines in new tests.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6150741c075
Actually deal with windows newlines.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/33ce4b92678c
Deal with windows newlines in more places in the new tests.

Comment 16

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/502f1e7d287e
https://hg.mozilla.org/mozilla-central/rev/f4529232e0a8

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ad67ead1905a
Obviously grid test needs the same tweak.

Comment 18

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/b85f9a5c6236
https://hg.mozilla.org/mozilla-central/rev/e6150741c075
https://hg.mozilla.org/mozilla-central/rev/33ce4b92678c
https://hg.mozilla.org/mozilla-central/rev/ad67ead1905a

Comment 19

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 20

[Emil Ghitta, QA [:emilghitta]]

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.