Closed Bug 640158 Opened 14 years ago Closed 11 years ago

document.loadOverlay() reapplies localstore.rdf data (icon size changes from "small" to "large", causing deformed toolbar buttons)

Categories

(Core :: XUL, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
blocking2.0 --- Macaw+
status2.0 --- .1-fixed

People

(Reporter: ecfbugzilla, Assigned: Gijs)

References

Details

(Keywords: regression, Whiteboard: [fx4-rc-ridealong])

Attachments

(3 files, 1 obsolete file)

Bug 631491 appears to have created an upgrade issue for people who reuse their Firefox 3.x profile with Firefox 4. Steps to reproduce: 1. Run Firefox 3.x, customize toolbar and switch icon size once or twice to get large icons (makes sure iconsize="large" is persisted). 2. Now run Firefox 4 with the same profile. 3. Install Adblock Plus 1.3.3 from https://addons.mozilla.org/addon/adblock-plus/ and customize toolbar to move its icon from add-on bar into navigation toolbar (this merely makes the issue obvious). Note that you see a small ABP icon. 4. Now click star button (bookmark) twice to bring up bookmark options. Expected results: icon size stays the same Actual results: toolbar switches to large icons (iconsize attribute changes from "small" to "large") and Adblock Plus icon stretches all the other buttons in the toolbar. This regressed between 2011-03-03 and 2011-03-04 Minefield builds, local backout confirmed that bug 631491 caused this regression (or at least made the issue obvious). Requesting blocking2.0 flag - I guess that lots of users will hit this when migrating from Firefox 3.6.
Attached image Screenshot (deleted) —
Screenshot shows navigation toolbar with iconsize="large" after bringing up bookmark options - not pretty.
Can you reproduce this without adblock plus?
As I said - Adblock Plus only makes the issue visible. iconsize="large" is being set regardless of any extensions but without extensions there is no toolbar button that will respect this attribute.
Just reproduced with a clean profile, only installed DOM Inspector to check attributes. After "upgrading" from Firefox 3.6 to Firefox 4 I see iconsize="small" on the nav-bar toolbar, bringing up bookmark options changes this attribute into iconsize="large". Without any custom buttons this doesn't have any visual effects of course.
Looks like document.loadOverlay("chrome://browser/content/places/editBookmarkOverlay.xul", ...) from browser-places.js causes localstore.rdf to be read and applied a second time on the entire document. Moving to core:xul.
Component: General → XUL
Product: Firefox → Core
QA Contact: general → xptoolkit.widgets
Whiteboard: regression
Attached patch workaround (checked in) (deleted) — Splinter Review
Attachment #518047 - Flags: review?
Attachment #518047 - Flags: review? → review?(mak77)
Ew. I don't think this will cause a respin of the RC for the following reasons: - while visible, it requires some heavy customization which (presumably) can be undone to restore a good state - it requires non-standard configurations for displaying toolbar icons - we can fix it in a .x Hurts me to not block, hurts more that it came from a non-blocking fix that we probably just shouldn't have approved :(
blocking2.0: ? → .x+
Whiteboard: [fx4-rc-ridealong]
Comment on attachment 518047 [details] [diff] [review] workaround (checked in) The iconsize attribute is packing both a user choice and a theme override, wouldn't be healthier to have 2 separate attributes and put css rules in a proper order for override? At that point the theme override could still not be persisted, but it would not race with the user choice. Apart this, if this is not going to be an emergency patch, it's better to figure out the localstore re-apply issue than taking a workaround. If otherwise we go for the workaround in a RC respin, I think we should file a bug to proper fix the underlying issue, annotate in that bug that we should remove this workaround once fixed, and have a comment like: // TODO (bug 123456): loading an overlay should not re-apply // localstore.rdf to the whole document. The r+ is conditioned to this case, if the respin won't happen we should directly patch the underlying issue.
Attachment #518047 - Flags: review?(mak77) → review+
(In reply to comment #8) > The iconsize attribute is packing both a user choice and a theme override, > wouldn't be healthier to have 2 separate attributes and put css rules in a > proper order for override? There are too many extensions using the iconsize attribute, so we don't want to change that unless absolutely necessary. > Apart this, if this is not going to be an emergency patch, it's better to > figure out the localstore re-apply issue than taking a workaround. For the 2.0 branch I think we only want the workaround, regardless of whether it's going to land for 4.0 or a dot release. > If otherwise we go for the workaround in a RC respin, I think we should file a > bug to proper fix the underlying issue I'm not going to close this bug.
(In reply to comment #9) > > Apart this, if this is not going to be an emergency patch, it's better to > > figure out the localstore re-apply issue than taking a workaround. > > For the 2.0 branch I think we only want the workaround, regardless of whether > it's going to land for 4.0 or a dot release. yeah, fine for stable branch.
Attachment #518047 - Flags: approval2.0?
Whiteboard: [fx4-rc-ridealong] → [fx4-rc-ridealong] fixed-in-cedar
Assignee: nobody → dao
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [fx4-rc-ridealong] fixed-in-cedar → [fx4-rc-ridealong]
Attachment #518047 - Attachment description: workaround → workaround (checked in)
Assignee: dao → nobody
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → NEW
Why was this bug reopened? Is the fix landed here not enough?
(In reply to comment #13) > Why was this bug reopened? Is the fix landed here not enough? It's a workaround, it doesn't solve the underlying problem.
blocking2.0: .x+ → Macaw
Keywords: regression
Comment on attachment 518047 [details] [diff] [review] workaround (checked in) Approved for mozilla2.0 repository, a=dveditz for release-drivers
Attachment #518047 - Flags: approval2.0? → approval2.0+
Whiteboard: [fx4-rc-ridealong] → [fx4-rc-ridealong][not-ready-for-cedar]
A user reported that this issue is happening on each browser restart for him. I guess that some extension is triggering it when the browser window opens.
(In reply to comment #18) > A user reported that this issue is happening on each browser restart for him. I > guess that some extension is triggering it when the browser window opens. I am that user, and it appears that the retrigger-on-browser-restart issue happens with a combination of Adblock Plus 1.3.6 RC, and Test Pilot 1.1. If I use Adblock Plus 1.3.6 RC without Test Pilot 1.1, and have the ABP button on the same row as the Back/Forward buttons, and do not "Use Small Icons", then on browser restart, the sizing of the row still looks good. If I use Adblock Plus 1.3.6 RC WITH Test Pilot 1.1, and have the ABP button on the same row as the Back/Forward buttons, and do not "Use Small Icons", then on browser restart, the size of the row becomes too tall, and the Back/Forward buttons look too vertically stretched (since the whole row is vertically stretched.)
I can confirm that Test Pilot 1.1 triggers this issue. It calls method TestPilotUIBuilder.buildCorrectInterface() when the browser window loads and that function then calls window.document.loadOverlay() to apply one of its two browser window overlays.
Summary: Clicking star button changes toolbar's icon size from "small" to "large" → document.loadOverlay() reapplies localstore.rdf data (e.g. clicking star button changes toolbar's icon size from "small" to "large")
I think this one should block next release, if the problem can be activated by any add-on using loadOverlay in the browser window.
blocking-fx: --- → ?
blocking-fx: ? → ---
Summary: document.loadOverlay() reapplies localstore.rdf data (e.g. clicking star button changes toolbar's icon size from "small" to "large") → document.loadOverlay() reapplies localstore.rdf data causing deformed toolbar buttons (icon size changes from "small" to "large")
Summary: document.loadOverlay() reapplies localstore.rdf data causing deformed toolbar buttons (icon size changes from "small" to "large") → document.loadOverlay() reapplies localstore.rdf data (icon size changes from "small" to "large", causing deformed toolbar buttons)
Where's the Macaw download? I thought it was first supposed to be released on Tuesday then got pushed to today (Thursday). Patches for this and bug 645551 are ready to land, but the Firefox 4 download page still has 4.0.0. So, what's the big wait for TWO patches?
In which version of Firefox will this patch land?
there is no patch for the underlying issue yet (otherwise it would be resolved fixed), the workaround is on all versions from 4.0 on.
Ok, how do I get this workaround? Im on the beta channel and still having this issue.
you already have it, the workaround fixes a well known issue, but not all of them, otherwise this bug would be fixed. Some add-on is known to cause this bug still.
Why did I not see this before sending in my report? Is bug 681143 a duplicate of the above discussed problem?
Whiteboard: [fx4-rc-ridealong][not-ready-for-cedar] → [fx4-rc-ridealong]
(In reply to Wladimir Palant from comment #20) > I can confirm that Test Pilot 1.1 triggers this issue. It calls method > TestPilotUIBuilder.buildCorrectInterface() when the browser window loads and > that function then calls window.document.loadOverlay() to apply one of its > two browser window overlays. I have disabled Test Pilot 1.2 in Mozilla Aurora 10 and can confirm this fixes my issue. Prior to this, even customizing the toolbar and resetting everything did nothing. Toggling small icons fixed the issue for a moment as opening a new window always brought the issue back. As for old profile regressions, my profile is a fresh one from Aurora 9. Everything is then drawn from my Weave account which overwrites my existing default profile. I don't know why this is happening but of all my addons, disabling Test Pilot 1.2 is the only one that reliably stopped these large icons from coming back.
Blocks: 737158
Depends on: 747310
Blocks: 747310
No longer depends on: 747310
I confirm that, like Adrian in Comment 33, I faced the same "big module icon" issue while installing Firefox 20 beta i.e. TestPilot. Even when manually resized to small, the issue rehappened after a browser restart. Disabling TesPilot module and restarting the browser immediately makes all module icons of the toolbar smaller (tested with AdBlockPlus and GreaseMonkey icons).
Blocks: 943785
So... I am a stranger in content/xul land, but I believe this fixes the bug we're running into. Of course, we could just ignore persisted data after the first document load, but I figured that we do want content in overlays loaded through loadOverlay to have persisted data... so I implemented something slightly more complicated. The good news being that I did also write a test for it, and it seems to work. Try push: https://tbpl.mozilla.org/?tree=Try&rev=3972b191dda4
Attachment #8342436 - Flags: review?(bzbarsky)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
I swear bzexport should warn before uploading empty try push patches.
Attachment #8342437 - Flags: review?(bzbarsky)
Attachment #8342436 - Attachment is obsolete: true
Attachment #8342436 - Flags: review?(bzbarsky)
Comment on attachment 8342437 [details] [diff] [review] restrict restoring persisted attributes when loading overlays after the first document load, Seems reasonable, but please brace single-line if bodies in this code. r=me
Attachment #8342437 - Flags: review?(bzbarsky) → review+
Comment on attachment 8342437 [details] [diff] [review] restrict restoring persisted attributes when loading overlays after the first document load, (In reply to Boris Zbarsky [:bz] from comment #37) > Comment on attachment 8342437 [details] [diff] [review] > restrict restoring persisted attributes when loading overlays after the > first document load, > > Seems reasonable, but please brace single-line if bodies in this code. > > r=me Landed with this nested one fixed: >diff --git a/content/xul/document/src/XULDocument.cpp b/content/xul/document/src/XULDocument.cpp >@@ -2971,16 +2982,24 @@ XULDocument::ResumeWalk() >+ // If we're only restoring persisted things on >+ // some elements, store the ID here to do that. >+ if (mRestrictPersistence) { >+ nsIAtom* id = child->GetID(); >+ if (id) >+ mPersistenceIds.PutEntry(nsDependentAtomString(id)); >+ } >+ But not this one: >@@ -2219,16 +2227,19 @@ XULDocument::ApplyPersistentAttributesIn > continue; > > nsAutoString id; > nsXULContentUtils::MakeElementID(this, nsDependentCString(uri), id); > > if (id.IsEmpty()) > continue; > >+ if (mRestrictPersistence && !mPersistenceIds.Contains(id)) >+ continue; >+ > // This will clear the array if there are no elements. > GetElementsForID(id, elements); > > if (!elements.Count()) > continue; > > ApplyPersistentAttributesToElements(resource, elements); > } Because all the other if statements in that block use the non-brace style. I'm *guessing* the top one was really what you meant; so in the spirit of asking for forgiveness rather than permission, I've landed as such. Please let me know if I was wrong. :-) remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/8853618ddaa8
Dão, the workaround has been removed in Australis because the persistent icon sizes are gone there. Assuming the general patch sticks and goes through to Holly, do you think it's worth backing out the workaround there? And do you know, offhand, if we have other workarounds in place for this bug that need to be cleared up? A MXR search for this bug number yielded nothing, but that might not be all there is to this...
Flags: needinfo?(dao)
(In reply to :Gijs Kruitbosch from comment #39) > Dão, the workaround has been removed in Australis because the persistent > icon sizes are gone there. Assuming the general patch sticks and goes > through to Holly, do you think it's worth backing out the workaround there? I don't think it's worth it, as the workaround shouldn't have a negative effect even if it's redundant. > And do you know, offhand, if we have other workarounds in place for this bug > that need to be cleared up? A MXR search for this bug number yielded > nothing, but that might not be all there is to this... I don't think there are other workarounds. If I had added one, I would likely have added a comment with this bug number.
Flags: needinfo?(dao)
Comment on attachment 8342437 [details] [diff] [review] restrict restoring persisted attributes when loading overlays after the first document load, (In reply to Dão Gottwald [:dao] from comment #40) > (In reply to :Gijs Kruitbosch from comment #39) > > Dão, the workaround has been removed in Australis because the persistent > > icon sizes are gone there. Assuming the general patch sticks and goes > > through to Holly, do you think it's worth backing out the workaround there? > > I don't think it's worth it, as the workaround shouldn't have a negative > effect even if it's redundant. > > > And do you know, offhand, if we have other workarounds in place for this bug > > that need to be cleared up? A MXR search for this bug number yielded > > nothing, but that might not be all there is to this... > > I don't think there are other workarounds. If I had added one, I would > likely have added a comment with this bug number. Makes sense, thanks for the quick response.
Attachment #8342437 - Flags: checkin+
Status: ASSIGNED → RESOLVED
Closed: 14 years ago11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: