Closed Bug 1528743 Opened 5 years ago Closed 5 years ago

The page 'jumps' when selecting text in a zoomed in state

Categories

(Core :: Panning and Zooming, defect, P2)

Product:
Core
Component:
Panning and Zooming
Version:
67 Branch
Platform:
ARM
Android
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox66 --- disabled
firefox67 + fixed

People

(Reporter: Laurentiu.Apahidean, Assigned: botond)

References

Details

Attachments

(1 file)

Bug 1528743 - Move the top-layer display items inside the async zoom container. r=mstange
47 bytes, text/x-phabricator-request
Details | Review

Steps to reproduce:

  1. In about:config set "layout.scroll.root-frame-containers" to "0".
  2. Go to a website that supports zooming in and text selection (Ex. https://en.wikipedia.org/wiki/Mozilla)
  3. Zoom in the page by double taping or pinch zoom.
  4. Long tap a word in order to select it.

Actual results:
The page focus shifts.

Expected results:
The word is selected and the page remains in the same position.

Notes: for more details please see the attached video.

The layer for the text selection handles carries RCD-RSF scroll metadata, but is not a descendant of the async zoom container. That's going to upset all sorts of APZ assumptions.

cc Markus

Here is a stack trace for the creation of a display item for the selection carets:

#0  nsDisplayItem::nsDisplayItem (this=0x7fc4db1ca420, aBuilder=0x7fc4db3ba000, aFrame=0x7fc4e182e528, aActiveScrolledRoot=0x7fc4e369cb00, aAnonymous=false)
    at /home/botond/dev/projects/mozilla/central/layout/painting/nsDisplayList.cpp:3077
#1  0x00007fc4f158ae73 in nsDisplayHitTestInfoItem::nsDisplayHitTestInfoItem (this=0x7fc4db1ca420, aBuilder=0x7fc4db3ba000, aFrame=0x7fc4e182e528, 
    aActiveScrolledRoot=0x7fc4e369cb00, aAnonymous=false) at /home/botond/dev/projects/mozilla/central/layout/painting/nsDisplayList.h:3603
#2  0x00007fc4f155f0fb in nsDisplayWrapList::nsDisplayWrapList (this=0x7fc4db1ca420, aBuilder=0x7fc4db3ba000, aFrame=0x7fc4e182e528, aList=0x7ffd49a0f160, 
    aActiveScrolledRoot=0x7fc4e369cb00, aClearClipChain=true, aIndex=0, aAnonymous=false)
    at /home/botond/dev/projects/mozilla/central/layout/painting/nsDisplayList.cpp:5492
#3  0x00007fc4f12cd78f in MakeDisplayItem<nsDisplayWrapList, nsIFrame*&, nsDisplayList*&, mozilla::ActiveScrolledRoot const*&, bool>(nsDisplayListBuilder*, nsIFrame*&, nsDisplayList*&, mozilla::ActiveScrolledRoot const*&, bool&&) (aBuilder=0x7fc4db3ba000, 
    at /home/botond/dev/projects/mozilla/central/layout/painting/nsDisplayList.h:2030
#4  0x00007fc4f12114d0 in WrapInWrapList (aBuilder=0x7fc4db3ba000, aFrame=0x7fc4e182e528, aList=0x7ffd49a0f160, aContainerASR=0x7fc4e369cb00, aCanSkipWrapList=false)
    at /home/botond/dev/projects/mozilla/central/layout/generic/nsFrame.cpp:3395
#5  0x00007fc4f1210bb8 in nsIFrame::BuildDisplayListForChild (this=0x7fc4e182e3b8, aBuilder=0x7fc4db3ba000, aChild=0x7fc4e182e5e0, aLists=..., aFlags=4)
    at /home/botond/dev/projects/mozilla/central/layout/generic/nsFrame.cpp:3794
#6  0x00007fc4f11ab7fe in DisplayLine (aBuilder=0x7fc4db3ba000, aLineArea=..., aLine=..., aDepth=0, aDrawnLines=@0x7ffd49a0f7ec: 32708, aLists=..., aFrame=0x7fc4e182e3b8, 
    aTextOverflow=0x0, aLineNumberForTextOverflow=0) at /home/botond/dev/projects/mozilla/central/layout/generic/nsBlockFrame.cpp:6405
#7  0x00007fc4f11aac96 in nsBlockFrame::BuildDisplayList (this=0x7fc4e182e3b8, aBuilder=0x7fc4db3ba000, aLists=...)
    at /home/botond/dev/projects/mozilla/central/layout/generic/nsBlockFrame.cpp:6496
#8  0x00007fc4f120cc7f in nsIFrame::BuildDisplayListForStackingContext (this=0x7fc4e182e3b8, aBuilder=0x7fc4db3ba000, aList=0x7ffd49a109b0, 
    aCreatedContainerItem=0x7ffd49a108d7) at /home/botond/dev/projects/mozilla/central/layout/generic/nsFrame.cpp:3061
#9  0x00007fc4f12105c0 in nsIFrame::BuildDisplayListForChild (this=0x7fc4e1ef0448, aBuilder=0x7fc4db3ba000, aChild=0x7fc4db3f7de8, aLists=..., aFlags=4)
    at /home/botond/dev/projects/mozilla/central/layout/generic/nsFrame.cpp:3711
#10 0x00007fc4f11ab7fe in DisplayLine (aBuilder=0x7fc4db3ba000, aLineArea=..., aLine=..., aDepth=0, aDrawnLines=@0x7ffd49a1103c: 32708, aLists=..., aFrame=0x7fc4e1ef0448, 
    aTextOverflow=0x0, aLineNumberForTextOverflow=0) at /home/botond/dev/projects/mozilla/central/layout/generic/nsBlockFrame.cpp:6405
#11 0x00007fc4f11aac96 in nsBlockFrame::BuildDisplayList (this=0x7fc4e1ef0448, aBuilder=0x7fc4db3ba000, aLists=...)
    at /home/botond/dev/projects/mozilla/central/layout/generic/nsBlockFrame.cpp:6496
#12 0x00007fc4f120cc7f in nsIFrame::BuildDisplayListForStackingContext (this=0x7fc4e1ef0448, aBuilder=0x7fc4db3ba000, aList=0x7ffd49a11ff8, aCreatedContainerItem=0x0)
    at /home/botond/dev/projects/mozilla/central/layout/generic/nsFrame.cpp:3061
#13 0x00007fc4f1185d15 in BuildDisplayListForTopLayerFrame (aBuilder=0x7fc4db3ba000, aFrame=0x7fc4e1ef0448, aList=0x7ffd49a122c8)
    at /home/botond/dev/projects/mozilla/central/layout/generic/ViewportFrame.cpp:124
#14 0x00007fc4f11859be in mozilla::ViewportFrame::BuildDisplayListForTopLayer (this=0x7fc4dee8d020, aBuilder=0x7fc4db3ba000, aList=0x7ffd49a122c8)
    at /home/botond/dev/projects/mozilla/central/layout/generic/ViewportFrame.cpp:180
#15 0x00007fc4f1185334 in mozilla::ViewportFrame::BuildDisplayList (this=0x7fc4dee8d020, aBuilder=0x7fc4db3ba000, aLists=...)
    at /home/botond/dev/projects/mozilla/central/layout/generic/ViewportFrame.cpp:64
#16 0x00007fc4f120cc7f in nsIFrame::BuildDisplayListForStackingContext (this=0x7fc4dee8d020, aBuilder=0x7fc4db3ba000, aList=0x7ffd49a13008, aCreatedContainerItem=0x0)
    at /home/botond/dev/projects/mozilla/central/layout/generic/nsFrame.cpp:3061
#17 0x00007fc4f1525508 in RetainedDisplayListBuilder::AttemptPartialUpdate (this=0x7fc4db3ba000, aBackstop=0, aChecker=0x0)
    at /home/botond/dev/projects/mozilla/central/layout/painting/RetainedDisplayListBuilder.cpp:1304
#18 0x00007fc4f10ecfcf in nsLayoutUtils::PaintFrame (aRenderingContext=0x0, aFrame=0x7fc4dee8d020, aDirtyRegion=..., aBackstop=0, 
    aBuilderMode=nsDisplayListBuilderMode::PAINTING, 
    aFlags=(nsLayoutUtils::PaintFrameFlags::PAINT_WIDGET_LAYERS | nsLayoutUtils::PaintFrameFlags::PAINT_EXISTING_TRANSACTION | nsLayoutUtils::PaintFrameFlags::PAINT_NO_COMP

The ASR that gets assigned to the item (which is the RCD-RSF's ASR, hence the layer getting the RCD-RSF metadata) gets pushed onto the display list builder in BuildDisplayListForTopLayerFrame().

I am seeing some bad behavior when in this zoomed in state besides the viewport changes. When in the state described by the bug I am not able to pan to the right to the selected text. Pinch zooming is broken. Pressing the device's back button allows the user to escape this broken state. https://www.youtube.com/watch?v=DFXONZHhZcI

Kevin: to clarify, you are testing with layout.scroll.root-frame-containers=0 (which is not the default yet)?

If so, then yes, based on the diagnosis in comment 1, I would expect all sorts of brokenness in the described state.

Yes I changed the pref. Is 0 the planned default?

(In reply to Kevin Brosnan [:kbrosnan] from comment #5)

Is 0 the planned default?

Yes, but only after this bug is fixed :)

Priority: P3 → P2
status-firefox66: --- → disabled

Yes, it sounds like the top layer needs to be moved inside the async zoom container. This can probably be achieved by doing the work that's currently done by ViewportFrame::BuildDisplayList somewhere in ScrollFrameHelper::BuildDisplayList instead, for root scroll frames. (Or maybe (equivalently?) for scroll frames that are viewport frames.)

At the moment, the top layer is on top of the root scroll frame's scroll bars. I think it would make sense to change that, and move it just underneath the scroll bars. Unless that means that, for example on YouTube, fullscreen video would display scrollbars from the page... I don't think it would though.

It may also make sense to build the display items for the top layer while the root scroll frame's asrSetter is on the stack, since top layer elements such as the carets or devtools highlighters can be scrolled. Actually, I don't really understand how that works at all, at the moment - where are those scrolled top layer items getting the right ASR from, if they're created after the root scroll frame's asrSetter has been popped from the stack?

In Chrome, which supports the dialog element and its ::backdrop pseudo element, I was able to achieve a scrolled element inside the top layer by setting position: absolute on the dialog's backdrop: https://position-absolute-backdrop.glitch.me (remix at https://glitch.com/edit/#!/position-absolute-backdrop )
Chrome places the scrollbars on top of that dialog backdrop.

The Firefox implementation of dialog::backdrop was happening in bug 1322939 but then stalled.

(In reply to Markus Stange [:mstange] from comment #7)

It may also make sense to build the display items for the top layer while the root scroll frame's asrSetter is on the stack, since top layer elements such as the carets or devtools highlighters can be scrolled. Actually, I don't really understand how that works at all, at the moment - where are those scrolled top layer items getting the right ASR from, if they're created after the root scroll frame's asrSetter has been popped from the stack?

I believe I answered that at the bottom of comment 2:

The ASR that gets assigned to the item (which is the RCD-RSF's ASR, hence the layer getting the RCD-RSF metadata) gets pushed onto the display list builder in BuildDisplayListForTopLayerFrame().

It's not clear to me what the implications of comment 8 are for the approach here. Could you elaborate?

(Otherwise, the approach described in comment 7 sounds reasonable, and I'm happy to give it a try.)

Assignee: nobody → botond

This maintains the important invariant that layers that carry scroll metadata
for the RCD-RSF are inside the async zoom container.

Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c5482772d73a
Move the top-layer display items inside the async zoom container. r=mstange

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

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox67: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Blocks: 1533607

Hello,

I tested this issue on the latest version of Nightly 67.0a1 (2019-03-13) with Sony Xperia Z3 (Android 5.1.1), Samsung Galaxy S8+ (Android 8.0.0), Huawei MediaPad M3 Lite 10 Android 7.0 and I wasn't able to reproduce the issue.

Due to my findings, I'll mark this issue as verified.

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

Attachment

General

Creator:
Created:
Updated:
Size: