Remove the fullscreenflex

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Toolbars and Customization
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

Trunk
Firefox 14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
Created attachment 609147 [details] [diff] [review]
patch

It's useless.
Attachment #609147 - Flags: review?(felipc)
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
Was it ever useful? Some more context here would be nice.
(Assignee)

Comment 2

6 years ago
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.
(Assignee)

Comment 3

6 years ago
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.

Updated

6 years ago
Blocks: 739160
(Assignee)

Comment 4

6 years ago
I should probably remove "#TabsToolbar > " here: http://hg.mozilla.org/mozilla-central/annotate/3e4735893504/browser/themes/winstripe/browser.css#l1151
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
(Assignee)

Comment 6

6 years ago
(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 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
Attachment #609147 - Flags: review?(felipc) → review+
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.
(Assignee)

Comment 9

6 years ago
https://hg.mozilla.org/mozilla-central/rev/4f8b9ff09a8a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
(Assignee)

Comment 10

6 years ago
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.
Attachment #609147 - Flags: approval-mozilla-aurora?

Comment 11

5 years ago
(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?
(Assignee)

Comment 12

5 years ago
> 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 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.
Attachment #609147 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 14

5 years ago
http://hg.mozilla.org/releases/mozilla-aurora/rev/4874cec6f1c8
status-firefox13: --- → fixed
Whiteboard: [qa-]
(Assignee)

Updated

5 years ago
Blocks: 782043
You need to log in before you can comment on or make changes to this bug.