Closed Bug 1313863 Opened 8 years ago Closed 8 years ago

[non-e10s regression] Page is not zoomed correctly after dragging a tab into another window

Categories

(Firefox :: Tabbed Browser, defect)

Product:
Firefox
Component:
Tabbed Browser
Version:
48 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox49 --- wontfix
firefox-esr45 --- unaffected
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- verified

People

(Reporter: JanH, Assigned: kmag)

References

Details

(Keywords: regression)

Attachments

(1 file)

Bug 1313863: Copy progress state flags when moving tabs between windows.
(deleted), text/x-review-board-request
Gijs
: review+
mconley
: review+
jcristau
: approval-mozilla-aurora+
Details
Tested on Windows with the current Nightly and also (for non-e10s) the release version (49.0.2). STR: 1. Open two Firefox windows (either both e10s, or both non-e10s). 2. Open any web page in a new tab in one of the windows and then change the page's zoom level. 3. Drag that tab into the other window. Expected: 1. The tab appears in the new window using the zoom level previously set and (in Nightly only) with the zoom level indicator in the location bar showing. Actual (non-e10s): The page is loaded at the default zoom level (100 %) with no zoom level showing in the location bar. After a reload the page then loads with the previously set zoom level and the indication showing in the location bar. Actual (e10s): The zoom level of the page itself is preserved, however the indicator in the location bar doesn't appear until you reload the page.
The 1st problem(non-e10s) is a new recent regression since 48. Regression window: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=ef9c1cc0a76ca05d459cda0a0d74266f5d916341&tochange=f47b31f50a9a82984640ff6e17a86239808c8e15 Regressed by: Bug 1238310 The 2nd problem(e10s) seems to be implementation problem of Bug 565718.
Blocks: 1238310
status-firefox49: --- → affected
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox-esr45: --- → unaffected
Flags: needinfo?(kmaglione+bmo)
Keywords: regression
See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=1089246
Issue 2 is bug 1300376. Morphing the bug title to reflect issue 1.
See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=1300376
Summary: Zoom level doesn't transfer correctly when dragging a tab into another window → [non-e10s regression] Page is not zoomed correctly after dragging a tab into another window
status-firefox49: affected → wontfix
status-firefox53: --- → affected
Version: Trunk → 48 Branch
Assignee: nobody → kmaglione+bmo
Flags: needinfo?(kmaglione+bmo)
It looks like this only ever worked before because it was winning a race that it's been losing since bug 1238310 landed. This patch fixes the code path in the drag code that's meant to handle it reliably.
Attachment #8824224 - Flags: review?(mconley)
Comment on attachment 8824224 [details] Bug 1313863: Copy progress state flags when moving tabs between windows. https://reviewboard.mozilla.org/r/102734/#review103350 ::: browser/base/content/tabbrowser.xml:544 (Diff revision 1) > <parameter name="aStartsBlank"/> > <parameter name="aWasPreloadedBrowser"/> > + <parameter name="aStateFlags"/> > <body> > <![CDATA[ > - let stateFlags = 0; > + let stateFlags = aStateFlags || 0; I'd like a second opinion from mconley, because I'm worried that it might be wrong and/or get clobbered, and mess things up, if we're swapping loaders into a new window that has to switch the remote type of the content process. ::: browser/base/content/tabbrowser.xml:551 (Diff revision 1) > stateFlags = Ci.nsIWebProgressListener.STATE_STOP | > Ci.nsIWebProgressListener.STATE_IS_REQUEST; We're still overriding this for preloaded browsers... can this happen for preloaded about:newtab things and in that case, what happens? ::: browser/base/content/test/general/browser_tab_drag_drop_perwindow.js:203 (Diff revision 1) > + "Original tab should have correct zoom factor"); > + > + let effect = EventUtils.synthesizeDrop(tab2, tab1, > + [[{type: TAB_DROP_TYPE, data: tab2}]], > + null, win2, win1); > + is(effect, "move", "Tab should be moved from win1 to win1."); Copy paste error?
Comment on attachment 8824224 [details] Bug 1313863: Copy progress state flags when moving tabs between windows. https://reviewboard.mozilla.org/r/102734/#review103352 Eh, meant to set r+ the first time, sorry for the spam.
Attachment #8824224 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8824224 [details] Bug 1313863: Copy progress state flags when moving tabs between windows. https://reviewboard.mozilla.org/r/102734/#review103350 > I'd like a second opinion from mconley, because I'm worried that it might be wrong and/or get clobbered, and mess things up, if we're swapping loaders into a new window that has to switch the remote type of the content process. We only allow moving tabs between windows where the remote/private type does not change. If we're swapping loaders, the state will always stay the same. > We're still overriding this for preloaded browsers... can this happen for preloaded about:newtab things and in that case, what happens? aWasPreloadedBrowser is never true for tabs being moved between windows, which is the only time we need to preserve state flags, so it doesn't really come into play.
Comment on attachment 8824224 [details] Bug 1313863: Copy progress state flags when moving tabs between windows. https://reviewboard.mozilla.org/r/102734/#review104286 This looks like the right call to me. If we flip remoteness, then we've aborted the load off of the network, and we will attempt it again once the remoteness flip is done, so there shouldn't be any state flags to clone. I also get the feeling that this might fix other unknown race-y tab tear out / tear in bugs.
Attachment #8824224 - Flags: review?(mconley) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc76b4cad2f917e0ef7c966f6b2846c01e56e05d Bug 1313863: Copy progress state flags when moving tabs between windows. r=Gijs,mconley
https://hg.mozilla.org/mozilla-central/rev/dc76b4cad2f9
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Hi Jan, can you please verify that the 12-Jan nightly is fixed for you?
status-firefox51: affected → wontfix
Flags: needinfo?(jh+bugzilla)
Comment on attachment 8824224 [details] Bug 1313863: Copy progress state flags when moving tabs between windows. Approval Request Comment [Feature/Bug causing the regression]: Bug 1238310 [User impact if declined]: This bug causes some inconsistencies in the handling of tabs dragged between windows. The particular visible effect reported in this bug is that zoom levels are not preserved after the tab is moved to the target window. [Is this code covered by automated tests?]: Yes. [Has the fix been verified in Nightly?]: No. [Needs manual test from QE? If yes, steps to reproduce]: Yes. STR are in comment 0. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: Low-risk. [Why is the change risky/not risky?]: The change simply preserves existing state information which is necessary to correctly manage the tab after it is moved to a new window. [String changes made/needed]: None.
Attachment #8824224 - Flags: approval-mozilla-aurora?
(In reply to Ryan VanderMeulen [:RyanVM] from comment #12) > Hi Jan, can you please verify that the 12-Jan nightly is fixed for you? Sure, will do.
Working fine now, thanks.
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
Flags: needinfo?(jh+bugzilla)
Comment on attachment 8824224 [details] Bug 1313863: Copy progress state flags when moving tabs between windows. fix zoom when moving a tab across windows, aurora52+
Attachment #8824224 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
needs rebasing for aurora
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(kmaglione+bmo)
Thanks :)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: