Closed Bug 977150 Opened 6 years ago Closed 6 years ago

Position of PanelUI-popup is hard coded

Categories

(Firefox :: Toolbars and Customization, defect)

29 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: mozbz, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P3-])

Attachments

(1 file)

The anchor position of PanelUI-popup is hard coded in PanelUI.show() as 'bottomcenter topright'.

An add-on to move the PanelUI button is very simple, but changing the position of the Panelmenu requires redefining the whole PanelUI.show function.

The position should be defined by a 'position' attribute, as with other panels and popups.
Whiteboard: [Australis:P3-]
This should work fine, and unifies some code, and fixes a subtle bug in the overflow panel.
Attachment #8383337 - Flags: review?(jaws)
Assignee: nobody → gijskruitbosch+bugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8383337 [details] [diff] [review]
don't hardcode position of Australis panels in JS; unify overflow show code,

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +3491,5 @@
>      }
>      let doc = this._panel.ownerDocument;
>      this._panel.hidden = false;
> +    let contextMenu = doc.getElementById(this._panel.getAttribute("context"));
> +    gELS.addSystemEventListener(contextMenu, 'command', this, true);

I take it the subtle bug you are hinting at is that if the panel was opened via some path other than clicking on the chevron, then the event listener would never be added. In the future it may be nice to say what the subtle bug that you're fixing is ;)
Attachment #8383337 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> Comment on attachment 8383337 [details] [diff] [review]
> don't hardcode position of Australis panels in JS; unify overflow show code,
> 
> Review of attachment 8383337 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/src/CustomizableUI.jsm
> @@ +3491,5 @@
> >      }
> >      let doc = this._panel.ownerDocument;
> >      this._panel.hidden = false;
> > +    let contextMenu = doc.getElementById(this._panel.getAttribute("context"));
> > +    gELS.addSystemEventListener(contextMenu, 'command', this, true);
> 
> I take it the subtle bug you are hinting at is that if the panel was opened
> via some path other than clicking on the chevron, then the event listener
> would never be added. In the future it may be nice to say what the subtle
> bug that you're fixing is ;)

Sorry, yes, that was what I meant. :-)
remote:   https://hg.mozilla.org/integration/fx-team/rev/29aef9e5c32f
Whiteboard: [Australis:P3-] → [Australis:P3-][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/29aef9e5c32f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3-][fixed-in-fx-team] → [Australis:P3-]
Target Milestone: --- → Firefox 30
Comment on attachment 8383337 [details] [diff] [review]
don't hardcode position of Australis panels in JS; unify overflow show code,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: add-ons can't easily control the appearance of the menu panel and/or overflow panel; small behavioural difference in closing behaviour of the overflow panel when it is programmatically opened
Testing completed (on m-c, etc.): on m-c 
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: low
Attachment #8383337 - Flags: approval-mozilla-aurora?
Attachment #8383337 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.