Lift per-browser viewport state from BrowserView into Tab and restore viewport state on Tabswitch

RESOLVED FIXED

Status

Fennec Graveyard
General
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: froystig, Unassigned)

Tracking

Details

Attachments

(1 attachment, 1 obsolete attachment)

15.11 KB, patch
Stuart Parmenter
: review+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
BrowserView keeps a BrowserViewportState on each browser element that has been set via setBrowser, but this prevents state from persisting if the browser element is removed, and also makes it difficult to restore viewport state on tab switch (e.g. scrolling to the visible position).

BrowserViewportState should live at the Tab level, be initialized upon the creation of the Tab's browser, and proper scrolling should be done on tab switch.

I fixed these in the attached patch.  There is still a minor bug in scrolling to the correct visible X position on tab switch caused by the fact that sideways chrome scrolling is incorrect in that, if a sidebar is open and the content viewport is large enough (e.g. content is zoomed), then scrolling the content viewport sideways doesn't move the sidebars off as it should, so Browser.getVisibleRect().left is not the most correct value always.  This will fix itself as soon as Browser.MainDragger is fixed.  Restoring visible Y works.
(Reporter)

Comment 1

9 years ago
To clarify, the issue with restoring the X position can be seen by opening a new tab, zooming in on content, scrolling to make sure a sidebar is in view (e.g. the tabs sidebar), switching to another tab, switching back, and the content should be slightly offset horizontally from expected location.
(Reporter)

Comment 2

9 years ago
Created attachment 393857 [details] [diff] [review]
patch

I should probably attach the patch.  Sorry for bugspam.
(Reporter)

Updated

9 years ago
Attachment #393857 - Flags: review?(pavlov)
(Reporter)

Comment 3

9 years ago
Created attachment 393862 [details] [diff] [review]
bvs_lift

Attaching patch with edits per review from Stuart.
Attachment #393857 - Attachment is obsolete: true
Attachment #393862 - Flags: review?(pavlov)
Attachment #393857 - Flags: review?(pavlov)

Updated

9 years ago
Attachment #393862 - Flags: review?(pavlov) → review+

Updated

9 years ago
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
http://hg.mozilla.org/mobile-browser/rev/d306f374f18f
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.