739093 - Remove the fullscreenflex

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

It's useless.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Was it ever useful? Some more context here would be nice.

Comment 2

[Dão Gottwald [:dao]]

It wasn't ever useful. Its only purpose is to push the fullscreen window controls to the end in case there are no other flexible elements on the toolbar.

Comment 3

[Dão Gottwald [:dao]]

http://forums.mozillazine.org/viewtopic.php?p=11855321#p11855321 motivated me to do this. As it happens, I've seen the back/forward buttons fail to connect with the location bar for no apparent reason on my aunt's computer two days ago. Resetting the toolbars fixed it.

Comment 4

[Dão Gottwald [:dao]]

I should probably remove "#TabsToolbar > " here: http://hg.mozilla.org/mozilla-central/annotate/3e4735893504/browser/themes/winstripe/browser.css#l1151

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 609147 [details] [diff] [review]
patch

Review of attachment 609147 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/nsBrowserGlue.js
@@ -1268,5 @@
> -                                          "$1bookmarks-menu-button-container,fullscreenflex$2")
> -        }
> -        else {
> -          currentset += ",bookmarks-menu-button-container";
> -        }

Is removing this from the code correct? Even though fullscreenflex is going away, the purpose of this is to remove "fullscreenflex" from currentset when going from UI version 2 to 3, so if someone is on that upgrade path the code should still remove it.

Additionally, should we upgrade the UI version with this change to remove it again? FTR I have fullscreenflex in my currentset

Comment 6

[Dão Gottwald [:dao]]

(In reply to Felipe Gomes (:felipe) from comment #5)
> Comment on attachment 609147 [details] [diff] [review]
> patch
> 
> Review of attachment 609147 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/nsBrowserGlue.js
> @@ -1268,5 @@
> > -                                          "$1bookmarks-menu-button-container,fullscreenflex$2")
> > -        }
> > -        else {
> > -          currentset += ",bookmarks-menu-button-container";
> > -        }
> 
> Is removing this from the code correct? Even though fullscreenflex is going
> away, the purpose of this is to remove "fullscreenflex" from currentset when
> going from UI version 2 to 3, so if someone is on that upgrade path the code
> should still remove it.

The purpose of this code isn't to remove fullscreenflex but to add bookmarks-menu-button-container.

> Additionally, should we upgrade the UI version with this change to remove it
> again? FTR I have fullscreenflex in my currentset

It doesn't hurt to have it in the currentset, toolbars handle non-existing ids just fine.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 609147 [details] [diff] [review]
patch

Review of attachment 609147 [details] [diff] [review]:
-----------------------------------------------------------------

Oh that's true, that was not meant to remove fullscreenflex but to add the bookmarks menu. currentset will also get cleared up after any customization so that shouldn't be a worry.

And yeah we can remove #TabsToolbar > #window-controls

Comment 8

[:Felipe Gomes (needinfo for replies!)]

There's only this addon on AMO that relies on this element: https://addons.mozilla.org/en-US/firefox/addon/bug582139/?src=ss
So giving a heads up to Alice.

Comment 9

[Dão Gottwald [:dao]]

https://hg.mozilla.org/mozilla-central/rev/4f8b9ff09a8a

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 609147 [details] [diff] [review]
patch

I'd like to land this fairly simple patch (mostly code removal) on Aurora for bug 739160. The feature being broken was new in Firefox 10.

Comment 11

[rob64rock]

(In reply to Dão Gottwald [:dao] from comment #10)
> Comment on attachment 609147 [details] [diff] [review]
> patch
> 
> I'd like to land this fairly simple patch (mostly code removal) on Aurora
> for bug 739160. The feature being broken was new in Firefox 10.

Could they not land this fix on Fx 12 beta also? If not, What would be the ramifications if they did?

Comment 12

[Dão Gottwald [:dao]]

> Could they not land this fix on Fx 12 beta also?

Theoretically, yes. Don't know what the chances are to get it approved.

Comment 13

[Alex Keybl [:akeybl]]

Comment on attachment 609147 [details] [diff] [review]
patch

[Triage Comment]
Regression in Firefox 10 - approving for Aurora 13. I'm concerned that there's too little time left in Beta 12 to confidently determine that there is no add-on compatibility fallout (esp. outside of AMO). For that reason, let's let this land in FF13 first.

Comment 14

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/4874cec6f1c8