1802466 - New wpt failures in /css/css-writing-modes/forms/text-input-vertical-overflow-no-scroll.html

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

Syncing wpt PR 37154 found new untriaged test failures in CI

Tests Affected

Firefox-only failures

• /css/css-writing-modes/forms/text-input-vertical-overflow-no-scroll.html [wpt.fyi]
  • input[type=text] in vertical-lr: typing characters in input should not cause the page to scroll: FAIL
  • input[type=text] in vertical-rl: typing characters in input should not cause the page to scroll: FAIL
  • input[type=password] in vertical-lr: typing characters in input should not cause the page to scroll: FAIL
  • input[type=password] in vertical-rl: typing characters in input should not cause the page to scroll: FAIL
  • input[type=search] in vertical-lr: typing characters in input should not cause the page to scroll: FAIL
  • input[type=search] in vertical-rl: typing characters in input should not cause the page to scroll: FAIL
  • input[type=number] in vertical-lr: typing characters in input should not cause the page to scroll: FAIL
  • input[type=number] in vertical-rl: typing characters in input should not cause the page to scroll: FAIL

CI Results

Missing results from treeherder
GitHub PR Head

Notes

These updates will be on mozilla-central once bug 1802408 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.

Comment 1

[Daniel Holbert [:dholbert]]

You can see the issue here locally if you:

1. Visit the testcase ( https://wpt.live/css/css-writing-modes/forms/text-input-vertical-overflow-no-scroll.html )
2. Focus the textfield (bottom left corner)
3. Press and hold "1" (or any number key) to fill up the textfield with numerals & cause its text content to overflow.

If I do that, my whole browser viewport suddenly starts scrolling downwards, as if we're trying to keep the overflowing content (inside the text area) in-view in the overall browser viewport, or something. I assume that's the same "typing characters in input should not cause the page to scroll" issue that the failure-message is referring to.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: reduced testcase 1

So the issue here doesn't have to do with orthogonal flows; it's just related to what happens when you overflow a textfield that happens to have a vertical writing mode.

We fail to scroll that textfield to "keep up" with the added text, for some reason. We just keep the textfield scrolled at its initial position, despite the fact that we've overflowed it and we're continuing to add more characters at its end.

In the WPT that spawned this bug, the content that's overflowing off the end (instead of being scrolled-to-be-visible) produces the side effect of scrolling the top-level viewport, since our scroll-into-view operation succeeds at that level. But really, we shouldn't have anything to scroll-into-view at that level -- the textfield should be handling the overflow by scrolling its contents to make the newly added characters visible.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: reduced testcase 2

There's a related issue with just moving the caret into overflowing territory with arrow keys, too.

If I click near the beginning of one of the widgets in this testcase, and then try to use the arrow keys to scroll towards the end, there's no way to just move the caret character-by-character in a way that lets me see the overflowing content.

Downarrow does seem to move the caret, but once the caret reaches the end, we just draw it at the bottom of the textarea without actually visually scrolling the content.

In contrast: in Chrome, rightarrow moves the caret downwards, and it does successfully scroll the content when it reaches the overflowing content.

Comment 4

[Daniel Holbert [:dholbert]]

I can reproduce the same issues in reduced testcases 1 and 2 going back to bug 1099032 (which is where we enabled support for writing-mode).

So: this has been an issue with vertical-writing-mode textfields since forever, I guess.

Comment 5

[Daniel Holbert [:dholbert]]

This code is at least partly implicated -- it's how we determine the spot that we're going to scroll to here:
https://searchfox.org/mozilla-central/rev/e7b8d13b7513b6fbd97d69e882d7faeed05309d0/dom/base/Selection.cpp#3238-3240,3258-3265

// Figure out what node type we have, then get the
// appropriate rect for it's nodeOffset.
bool isText = node->IsText();
...
// Return the rect relative to the frame, with zero width.
if (isText) {
  aRect->x = pt.x;
} else if (mFrameSelection->GetHint() == CARET_ASSOCIATE_BEFORE) {
  // It's the frame's right edge we're interested in.
  aRect->x = frame->GetRect().Width();
}
aRect->SetHeight(frame->GetRect().Height());

The physical-axis-specific stuff (e.g. only setting aRect->x in the outparam) is likely part of the issue here.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: Bug 1802466 part 2: Change GetSelectionEndPointGeometry to handle vertical-writing-mode text nodes. r?#layout-reviewers

This function had some code that we hadn't yet updated to support logical axes
and vertical writing modes. This patch adds some clearer documentation to
describe the rect that we seem to be returning (now in logical-axis terms), and
this patch makes it more generic so that it can properly handle
vertical-writing-mode text nodes (so that we can e.g. properly keep the caret
scrolled into view, in a vertical-writing-mode textfield).

I'm just focusing on vertical vs. horizontal generification for now. I'm not
entirely sure if it makes sense to make it even-more-generic-still to support
reversed-inline-direction writing modes as well. I wasn't able to trigger any
issues with those in some brief testing, so I think this patch is sufficient
as-is, but I left a TODO(dholbert) code-comment to call out that ambiguity
that's still present in the code.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: Bug 1802466 part 1: Refactor computation of outparam rect in GetSelectionEndPointGeometry. r?#layout-reviewers

This patch doesn't change behavior. It's just laying the groundwork for the
next patch which adds vertical-writing-mode support.

(This patch adds some documentation in terms of logical axes, though, which
will become accurate as of the next patch in this series.)

Comment 8

[Daniel Holbert [:dholbert]]

So with the attached patch, we pass the test text-input-vertical-overflow-no-scroll.html on my local machine, but we fail on TreeHerder with an off-by-1px scroll position in some of the scrollTop comparisons. I think that failure depends on font & font-size & maybe other factors to trigger the off-by-1px issue (and then also you need a little bit of luck). I was able to trigger it locally after changing the font and the font-size; but it still wasn't 100% reproducible. It did interestingly seem to toggle between "rounded-up" vs. "rounded-down" when I backgrounded/foregrounded the window that the test was running in, which was a clue.

Looking at this off-by-1px failure on pernosco, I chased the pixel change back a bit and landed here:
https://searchfox.org/mozilla-central/rev/132ffbfd6842e5ecd3813673c24da849d3c9acf8/gfx/layers/FrameMetrics.cpp#52,59-62

void FrameMetrics::RecalculateLayoutViewportOffset() {
...
  // For the root, the two viewports can diverge, but the layout
  // viewport needs to keep enclosing the visual viewport.
  KeepLayoutViewportEnclosingVisualViewport(GetVisualViewport(),
                                            mScrollableRect, mLayoutViewport);

At this point in my pernosco run, I have:

mLayoutViewport.y = 617
GetVisualViewport().y = 616.016663

...and this^ difference ends up reducing the y-position of our mLayoutViewport to 616.016663 and generating a RepaintRequest with that updated mLayoutViewport which ultimately causes a change in the scroll position of our root frame from 617 to 616. And that's observable to the test.

I'm chasing that back further to see what more I can find out, but I wonder if that's something we can suppress or avoid by doing some sort of flushing, or by ensuring certain sizes or positions are whole-pixel-valued, or if there's any other recommended workaround to avoid this? (Or maybe it's a legit bug? It definitely seems a bit fiddly that this is producing ~arbitrary-seeming 1px changes to scrollTop without the user actually scrolling.)

Comment 9

[Daniel Holbert [:dholbert]]

Aha, I think the issue is that our <input> element itself has a fractional-pixel-value height (since it's just sized based on a default number-of-characters for the given font, which might produce a fractional size). And that means the test's scrollIntoView call ends up scrolling to a fractional pixel value, and then that produces some 1px nondeterminism / ping-ponging from that point on, apparently.

That nondeterminism/ping-ponging seems like a bug to me; I'll see if I can come up with a reduced testcase that demonstrates it, to spin off as a followup.

But it looks like we can avoid it here by setting a whole-pixel-valued size on the input element, which seems like a reasonable thing to do. If the test is checking the scroll position (using a whole-pixel-valued scrollTop API), it seems reasonable to do what we can to be sure we're not setting it to a fractional (font-dependent) position before we check it.

Comment 10

[Daniel Holbert [:dholbert]]

I spun off bug 1851066 for the off-by-1px thing discussed in comment 8 - comment 9 here.

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: Bug 1802466 part 3: Set an explicit whole-pixel-valued height on the textfield in text-input-vertical-overflow-no-scroll.html. r?ntim

This avoids triggering bug 1851066, per the comment in the test. Bug 1851066
is a real bug, but it's quite subtle and is unrelated to what this WPT is
actually trying to test.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/59c6e8943e53
part 1: Refactor computation of outparam rect in GetSelectionEndPointGeometry. r=emilio
https://hg.mozilla.org/integration/autoland/rev/9461b0e136d3
part 2: Change GetSelectionEndPointGeometry to handle vertical-writing-mode text nodes. r=emilio
https://hg.mozilla.org/integration/autoland/rev/1af6a9b30e2c
part 3: Set an explicit whole-pixel-valued height on the textfield in text-input-vertical-overflow-no-scroll.html. r=ntim

Comment 13

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

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

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f5599c15bc89
Backed out 3 changesets for mochitest failure on test_composition_text_querycontent.xhtml . CLOSED TREE

Comment 15

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

Upstream PR was closed without merging

Comment 16

[Narcis Beleuzu [:NarcisB]]

Please also check :
WPT failure on text-input-vertical-overflow-no-scroll.html
MN failure on test_accessiblecaret_selection_mode.py
Mochitest failure on test_scroll_selection_into_view.html
Mochitest failure on test_moving_and_expanding_selection_per_page.html
Mochitest failure on layout/base/tests/test_bug1714640.html

Comment 17

[Daniel Holbert [:dholbert]]

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

Please also check :
WPT failure on text-input-vertical-overflow-no-scroll.html

That's the test I'm fixing here. It looks like we're still triggering bug 1851066 there (the failure log shows an off-by-1px scrollTop test failure).

We can just annotate it as expected-fail on Android for now, I suppose. I don't want to add further hacks to that test just for an Android-specific issue, since it's in the common WPT repo.

The other failures are due to a typo that I made when addressing a review comment (whoops). I was able to reproduce the text failure in test_composition_text_querycontent.xhtml (the test mentioned in comment 14) locally, and then after fixing the typo, I can't reproduce any issues in that or the other failing tests locally. So I think those are all set with the typo fixed.

Comment 18

[Daniel Holbert [:dholbert]]

Try run with updated patch:
https://treeherder.mozilla.org/jobs?repo=try&revision=bf3ecd5fb8dfb075b45dcb44f9bb4d434d134c60

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dd39ef16226f
part 1: Refactor computation of outparam rect in GetSelectionEndPointGeometry. r=emilio
https://hg.mozilla.org/integration/autoland/rev/96c99259bbec
part 2: Change GetSelectionEndPointGeometry to handle vertical-writing-mode text nodes. r=emilio
https://hg.mozilla.org/integration/autoland/rev/fecbf97cc8db
part 3: Set an explicit whole-pixel-valued height on the textfield in text-input-vertical-overflow-no-scroll.html. r=ntim

Comment 20

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/dd39ef16226f
https://hg.mozilla.org/mozilla-central/rev/96c99259bbec
https://hg.mozilla.org/mozilla-central/rev/fecbf97cc8db

Comment 21

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

Upstream PR merged by moz-wptsync-bot