History panel opens anchored to menu button instead of anchored to history button

VERIFIED FIXED in Firefox 30

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: quicksaver, Assigned: Gijs)

Tracking

29 Branch
Firefox 32
Points:
---
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(firefox29 wontfix, firefox30+ verified, firefox31+ verified, firefox32+ verified)

Details

(Whiteboard: [good first verify] [testday-20140523])

Attachments

(2 attachments, 2 obsolete attachments)

Here's a non-life threatening, but still weird, one. Didn't check the code on this one so don't know exactly why this is happening.

STR:
1) Clean profile
2) Open customize
3) Drag history button to nav-bar
4) Hit "Restore defaults"
5) Hit "Undo"
6) Exit customize
7) Click the history button

Result: the panel opens anchored to the menu button instead of anchored to the history button.
Assignee

Comment 1

5 years ago
Ummm. Whoa. That's bad. :-\

I'm assuming this reproduces in 29/30/31/32 ? (I just checked in 32 and can definitely reproduce there)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: firefox-backlog+
I've tried it in all four, full set. :)
It sounds like Restore Defaults is clearing the anchor attribute in the process, and the Undo isn't placing it back on. Maybe the code which sets the anchor attribute isn't getting hit by the Undo code path?
Gijs says it is bad! Tracking!
Assignee

Comment 5

5 years ago
So this is exactly the other way around as the first sentence of comment #3, but comment #4 is essentially correct. setLocationAttributes makes an assumption that it only ever has to set the anchor, never remove it. That's true for nodes which are moved through the API, or initial builds, but not for resets or 'undoes'. That causes this particular bug. In fact, thinking about this more, I suppose that if you manually moved the bookmarks button to the main menu and then hit reset, you might hit the same bug. I'll doublecheck, and if so, add a test for that, too (the 2-line code fix here should fix that issue, too).
Attachment #8421090 - Flags: review?(mconley)
Assignee

Comment 6

5 years ago
(In reply to :Gijs Kruitbosch from comment #5)
> Created attachment 8421090 [details] [diff] [review]

> but comment #4 is essentially correct.

Err, I meant, the second sentence of comment #3. Sigh.
If I can, I have a technical question which isn't really specific to this bug, but I saw it in your patch and it reminded me (I've seen similar routines a few times before in other patches).

Why are you checking for hasAttribute() before removeAttribute()? Why not removeAttribute() directly since that will produce the same result? Does this somehow improve code execution, as in are the internal checks of removeAttribute() too many or unnecessary which can be simplified with a hasAttribute() explicit check before it (or if not, won't it become redundant internal double checking like this)?

Or is it just your personal, or mozilla's, method of keeping the code clean/organized?
Assignee

Comment 8

5 years ago
My suspicions were correct, so I've added a test for the bookmarks button and reset case.
Attachment #8421135 - Flags: review?(mconley)
Assignee

Updated

5 years ago
Attachment #8421090 - Attachment is obsolete: true
Attachment #8421090 - Flags: review?(mconley)
Assignee

Comment 9

5 years ago
(In reply to Luís Miguel [:Quicksaver] from comment #7)
> If I can, I have a technical question which isn't really specific to this
> bug, but I saw it in your patch and it reminded me (I've seen similar
> routines a few times before in other patches).
> 
> Why are you checking for hasAttribute() before removeAttribute()? Why not
> removeAttribute() directly since that will produce the same result? Does
> this somehow improve code execution, as in are the internal checks of
> removeAttribute() too many or unnecessary which can be simplified with a
> hasAttribute() explicit check before it (or if not, won't it become
> redundant internal double checking like this)?
> 
> Or is it just your personal, or mozilla's, method of keeping the code
> clean/organized?

I've seen guards like these (for both removeAttribute, but also setAttribute, in the following way:

if (foo.getAttribute(a) != "b") {
  foo.setAttribute("a", "b");
}

be justified by perf reasons. I've never actually checked if this is generally true, or just in specific circumstances, or if it's no longer true even if it maybe used to be.

I guess I might as well ask someone who stands a chance of actually knowing... Boris, can you clarify if this kind of thing helps/hurts perf in any way, or is just unnecessary (by now)? :-)
Flags: needinfo?(bzbarsky)
We should never have to guard 'removeAttribute' with a 'hasAttribute'. I remember a microbenchmark of Gecko a while back that showed that 'hasAttribute' is slower than 'removeAttribute', since 'removeAttribute' has been heavily optimized since it is used much more in practice.

As to calling 'getAttribute' and comparing the return value before calling 'setAttribute', see this old comment from dave hyatt: hg.mozilla.org/mozilla-central/annotate/3bb70c0d6845/browser/base/content/browser.js#l250
That comment is .... quite old.  ;)

Here's what Gecko will do nowadays on a setAttribute call:

1)  Check for an existing attr name that matches the passed in name.
2)  If there is none, check that the string passed in is a valid attr name, and if it is 
    atomize it (possibly after lowercasing, in HTML).
3)  Check whether there are mutation listeners.
4)  Get the old value.
5)  If the old value matches the new one and there are no mutation listeners and this is
    not a custom element, call AttributeSetToCurrentValue on all the mutation observers
    (largely to queue up mutation observers) and bail out.

getAttribute has to basically do item 1 from that list and a slightly faster version of item 4 (since it combines the two walks).  It has to do the additional work of converting the string to a JS string.  All of which is to say, need to measure.

Hence this testcase. It shows that the test-and-set-if-not-equal is a bit faster, at least on my (new laptop) hardware: ~40ns vs ~100ns.

On the other hand, unless you're doing quite a number of sets, the difference might not be worth the loss of code clarity...  In the days when hyatt wrote that code comment, just the XPConnect overhead of any DOM call at all was ~1100ns on today's hardware.
Flags: needinfo?(bzbarsky)
Assignee

Comment 13

5 years ago
While a microbenchmark based on bz's shows that a check is fractionally (<5%, couple of ns) faster than not checking before removeAttribute, it's not actually necessary. Sticking with Mike for review because I'm already waiting on several others from Jared... ;-)
Attachment #8421261 - Flags: review?(mconley)
Assignee

Updated

5 years ago
Attachment #8421135 - Attachment is obsolete: true
Attachment #8421135 - Flags: review?(mconley)
So, as a general rule, yay for guarding setAttribute, nay for guarding removeAttribute. Thanks for everyone's replies on this. :)
Comment on attachment 8421261 [details] [diff] [review]
setLocationAttributes, as called from buildArea, should remove anchor attribute if set,

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

LGTM!

::: browser/components/customizableui/test/browser_1008559_anchor_undo_restore.js
@@ +2,5 @@
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +const anchorAttribute = "cui-anchorid";

nit - consts should start with k, or be in ALL_CAPS.
Attachment #8421261 - Flags: review?(mconley) → review+
Assignee

Comment 16

5 years ago
w/ kAnchorAttribute

remote:   https://hg.mozilla.org/integration/fx-team/rev/bafa46fb7f18
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/bafa46fb7f18
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
Uplift nom?  Let's get this into Thursday's beta to get the most time with users before ship.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee

Comment 19

5 years ago
Comment on attachment 8421261 [details] [diff] [review]
setLocationAttributes, as called from buildArea, should remove anchor attribute if set,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: Under certain conditions, panels will open in the wrong places
Testing completed (on m-c, etc.): locally, on m-c, has automated test
Risk to taking this patch (and alternatives if risky): low, because of test coverage
String or IDL/UUID changes made by this patch: none
Attachment #8421261 - Flags: approval-mozilla-beta?
Attachment #8421261 - Flags: approval-mozilla-aurora?
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8421261 - Flags: approval-mozilla-beta?
Attachment #8421261 - Flags: approval-mozilla-beta+
Attachment #8421261 - Flags: approval-mozilla-aurora?
Attachment #8421261 - Flags: approval-mozilla-aurora+
Marking this as [good first verify] since it has testsuite coverage already, but also has good steps to reproduce.
Whiteboard: [good first verify]
Verified fixed using Windows 7 64 bit and Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0. Verified in latest Nightly as well, setting the STATUS to VERIFIED.
Status: RESOLVED → VERIFIED
Whiteboard: [good first verify] → [good first verify] [testday-20140523]
Hi, 

I was able to reproduce it on 32.0a1 2014-05-10 and I verified it on latest Nightly (32.0a1 2014-05-29), Aurora (31.0a2 2014-05-29) and Beta (30.0 2014-05-27) on Debian (Linux) x86_64. 
Also, verified on Win 7 x86_64 for Aurora (31.0a2 2014-05-29).

Cheers,
Francesca
You need to log in before you can comment on or make changes to this bug.