981305 - [Australis] Developer widget - Don't copy over <menuseparator> if previous item is the first item and a <menu> element

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Tim Nguyen :ntim]

Here 's my web developer menu :
Firebug     >
-------------
Toggle tools
Console
Inspector
Debugger
[...]

When the web developer menu gets copied over to the web developer widget, it will remove the <menu> elements for design issues. And my panel will look like this :

------------ (useless separator on the top of panel)
Toggle tools
[...]

I'll attach a screen so you can understand better the situation.

Comment 1

[Tim Nguyen :ntim]

Attachment: HTML file to show the problem

Comment 2

[Justin Dolske [:Dolske]]

P4 in terms of impact, but I'm going to call this a P3 because it involves Firebug and devtools, which are more important than average.

Comment 3

[Justin Dolske [:Dolske]]

(Also, you've been filing useful bugs, so I've set your canconfirm bit so you can file / switch bugs to the NEW state, instead of just UNCONFIRMED. Thanks!)

Comment 4

[:Gijs (he/him)]

Attachment: don't insert leading or duplicate separators,

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8394120 [details] [diff] [review]
don't insert leading or duplicate separators,

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

LGTM. Would be nice to have a test for this if you're feeling adventurous.

Comment 6

[:Gijs (he/him)]

Attachment: don't insert leading or duplicate separators,

Now with test goodness.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8394253 [details] [diff] [review]
don't insert leading or duplicate separators,

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

Just one nit, otherwise, looks good.

::: browser/components/customizableui/test/browser_981305_separator_insertion.js
@@ +41,5 @@
> +  PanelUI.hide();
> +  yield panelHiddenPromise;
> +});
> +
> +add_task(function asyncCleanup() {

Nothing async going on in here - might as well use registerCleanupFunction.

Comment 8

[Tim Nguyen :ntim]

(In reply to Justin Dolske [:Dolske] from comment #3)
> (Also, you've been filing useful bugs, so I've set your canconfirm bit so
> you can file / switch bugs to the NEW state, instead of just UNCONFIRMED.
> Thanks!)

Thanks :)

Comment 9

[Tim Nguyen :ntim]

Oh btw, the patch should also remove the separator if the separator is the last item.

Comment 10

[:Gijs (he/him)]

Comment on attachment 8394253 [details] [diff] [review]
don't insert leading or duplicate separators,

(In reply to Tim Nguyen [:ntim] from comment #9)
> Oh btw, the patch should also remove the separator if the separator is the
> last item.

Egh. This is a lot harder to fix. :-\

Comment 11

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #10)
> Comment on attachment 8394253 [details] [diff] [review]
> don't insert leading or duplicate separators,
> 
> (In reply to Tim Nguyen [:ntim] from comment #9)
> > Oh btw, the patch should also remove the separator if the separator is the
> > last item.
> 
> Egh. This is a lot harder to fix. :-\

Does it actually happen in practice? Do people insert menus at the bottom of that menu, with a separator?

Comment 12

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #10)
> Comment on attachment 8394253 [details] [diff] [review]
> don't insert leading or duplicate separators,
> 
> (In reply to Tim Nguyen [:ntim] from comment #9)
> > Oh btw, the patch should also remove the separator if the separator is the
> > last item.
> 
> Egh. This is a lot harder to fix. :-\

Err, actually, I was wrong - the patch actually already handles this! I've added a test bit to prove it, and will be landing that shortly. :-)

Comment 13

[:Gijs (he/him)]

w/ nits + extra test for the trailing separator case,

remote:   https://hg.mozilla.org/integration/fx-team/rev/9ea07591d94b

Comment 14

[Tim Nguyen :ntim]

Thanks for the fix ! 
It'd be nice if the sidebar widget could avoid similar problems too. I guess the fix will be similar :).

Comment 15

[:Gijs (he/him)]

(In reply to Tim Nguyen [:ntim] from comment #14)
> Thanks for the fix ! 
> It'd be nice if the sidebar widget could avoid similar problems too. I guess
> the fix will be similar :).

I looked at that code but didn't think that it'd ever have separators, nevermind separators and submenus. Are there add-ons that do that? :-(

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9ea07591d94b

Comment 17

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #15)
> (In reply to Tim Nguyen [:ntim] from comment #14)
> > Thanks for the fix ! 
> > It'd be nice if the sidebar widget could avoid similar problems too. I guess
> > the fix will be similar :).
> 
> I looked at that code but didn't think that it'd ever have separators,
> nevermind separators and submenus. Are there add-ons that do that? :-(

You should try All in One sidebar. It creates a duplicate separator.

Comment 18

[:Gijs (he/him)]

Filed bug 986808 for the sidebar case. It's less important because 29 isn't affected.

Comment 19

[:Gijs (he/him)]

Comment on attachment 8394253 [details] [diff] [review]
don't insert leading or duplicate separators,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: web developer subview can look ugly, with confusing/duplicate separators
Testing completed (on m-c, etc.): on m-c, has automated test
Risk to taking this patch (and alternatives if risky): very low
String or IDL/UUID changes made by this patch: none

Comment 20

[:Gijs (he/him)]

https://hg.mozilla.org/releases/mozilla-beta/rev/326aa82fb898
https://hg.mozilla.org/releases/mozilla-aurora/rev/a44f8b39c2c8

Comment 21

[Andrei Vaida [:avaida]]

I was able to confirm the fix for this bug on the latest Beta (Build ID: 20140324101726), latest Aurora (Build ID: 20140325004002) and latest Nightly (Build ID: 20140325030201), using:
- Windows 7 64-bit [1],
- Windows 8.1 64-bit [2],
- Ubuntu 13.10 64-bit [3],
- Mac OS X 10.9 [4].

1. Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:31.0) Gecko/20100101 Firefox/31.0
2. Mozilla/5.0 (Windows NT 6.3; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0
3. Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Firefox/31.0
4. Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Firefox/31.0