Closed Bug 979587 Opened 6 years ago Closed 6 years ago

Australis: sometimes bookmarks sidebar opens with bookmarks menu

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

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

People

(Reporter: RyanVM, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P2])

Attachments

(1 file, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #973217 +++

This is definitely improved since bug 973217 landed, but I've still hit it twice in the last day or so and Gijs tells me he has too.
Don't know how/why this is still broken. :-\
Whiteboard: [Australis:P?] → [Australis:P2]
(In reply to :Gijs Kruitbosch from comment #1)
> Don't know how/why this is still broken. :-\

Mike and I debugged this, because it's very reproducible on his machine (and not on mine).

Basically what seems to happen is the click event fires before the popupshown event, which means our pointerEvents disabling isn't working.

I have a cunning plan:

0) set style="pointer-events: none" on the popup;
1) remove that in popupshown, and do our current dance. Add an attribute to the popup indicating whether pointer-events should be disabled until the transitionend
2) in popupshowing, check for the attribute and if truthy, disable pointer-events
3) in popupshown, check for the attribute instead of the expensive getComputedStyle checks.

This is still pretty sad but on the upside, it /should/ work.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Ploink.
Attachment #8385708 - Flags: review?(mconley)
This still needs a removeProperty call for the case where we don't need to disable the events for the initial show.
Think I forgot the linux case which doesn't have a transition, but this should still work, I think? Please test. :-)
Attachment #8385809 - Flags: review?(mconley)
Attachment #8385708 - Attachment is obsolete: true
Attachment #8385708 - Flags: review?(mconley)
Comment on attachment 8385809 [details] [diff] [review]
attempt to fix sidebar opening from Australis bookmarks menu,

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

This didn't work at first on Ubuntu, but that's because you need to switch to using this.style.removeProperty when removing pointer-events property. I tried that locally, and it seemed to work just fine.

So r=me with that fixed.

::: browser/components/places/content/menu.xml
@@ +586,5 @@
> +          let container = document.getAnonymousElementByAttribute(this, "anonid", "container");
> +          let cs = getComputedStyle(container);
> +          let transitionProp = cs.transitionProperty;
> +          let transitionTime = parseFloat(cs.transitionDuration);
> +          disablePointerEvents = (transitionProp.indexOf("transform") > -1 ||

Can just use transitionProp.contains("transform") here.

@@ +594,5 @@
> +        } else {
> +          disablePointerEvents = this.getAttribute("disablepointereventsfortransition") == "true";
> +        }
> +        if (!disablePointerEvents) {
> +          this.removeProperty("pointer-events");

This needs to be this.style.removeProperty.
Attachment #8385809 - Flags: review?(mconley) → review+
remote:   https://hg.mozilla.org/integration/fx-team/rev/2c6243a37618
Whiteboard: [Australis:P2] → [Australis:P2][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/2c6243a37618
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P2][fixed-in-fx-team] → [Australis:P2]
Target Milestone: --- → Firefox 30
Comment on attachment 8385809 [details] [diff] [review]
attempt to fix sidebar opening from Australis bookmarks menu,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: bookmarks menu button doesn't respond consistently
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: none
Attachment #8385809 - Flags: approval-mozilla-aurora?
Attachment #8385809 - 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.