440702 - Link modifiers should use the command attribute instead of oncommand(and remove observes elements)

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Klaas Heidstra]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1a1pre) Gecko/2008062013 Minefield/3.1a1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1a1pre) Gecko/2008062013 Minefield/3.1a1pre

Middle-clicking on elements such as the back and forward buttons is currently made possible by using
> oncommand="BrowserForward(event)"
This should be replaced by:
> command="Browser:Forward"
And the command should be:
> <command id="Browser:Forward" oncommand="BrowserForward(event);" disabled="true"/>

Those elements also use:
> <observes element="Browser:Forward" attribute="disabled"/>
These aren't needed anymore so they can be removed.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Klaas Heidstra]

Attachment: patch - applies on top of bug263942

This also makes middle-click work with the back/forward menuitems in the context-menu. I set it to always ignore Alt just like bug263942, so with this patch it is not possible anymore to save a page by Alt-clicking the back/forward buttons.

Comment 2

[Klaas Heidstra]

Attachment: patch v2 - applies on top of bug 263942

In the previous patch I have forgotten to replace one oncommand attribute with command, that is corrected in this patch.

Comment 3

[Klaas Heidstra]

Attachment: patch v3 - applies on top of bug 263942

I set ignoreAlt to always be true for browserGoHome to so that it fixes bug 422732 completely now.

Comment 4

[Klaas Heidstra]

Attachment: patch v4 - applies on top of bug 263942

Update so that it applies to the latest patch in bug 263942.

Comment 5

[Klaas Heidstra]

Attachment: patch v5 - applies on top of bug 263942

Update again.

Comment 6

[Klaas Heidstra]

Attachment: patch v6 - applies on top of bug 263942

Update.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 326178 [details] [diff] [review]
patch v6 - applies on top of bug 263942

>-    <command id="Browser:Back"    oncommand="BrowserBack();" disabled="true"/>
>-    <command id="Browser:Forward" oncommand="BrowserForward();" disabled="true"/>
>+    <command id="Browser:Back"    oncommand="BrowserBack(event);" disabled="true"/>
>+    <command id="Browser:Forward" oncommand="BrowserForward(event);" disabled="true"/>

Similar to Ctrl+R in bug 263942, I think this is going to fail on Mac and Linux. See goBackKb, goForwardKb, goBackKb2 and goForwardKb2 in browser-sets.inc.

Comment 8

[Klaas Heidstra]

(In reply to comment #7)
> Similar to Ctrl+R in bug 263942, I think this is going to fail on Mac and
> Linux. See goBackKb, goForwardKb, goBackKb2 and goForwardKb2 in
> browser-sets.inc.

Do you think I should add a Browser:Back/ForwardOrDuplicate command similar to bug 263942?

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #8)
> Do you think I should add a Browser:Back/ForwardOrDuplicate command similar to
> bug 263942?

I'm not sure about the name, but basically it sounds like the cleanest solution to me.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

As I mentioned in bug 263942, I think supporting middle-click of menuitems is undesirable.

Comment 11

[Klaas Heidstra]

Attachment: patch v7 - applies on top of bug 263942

I'm now using BackOrBackDuplicate and ForwardOrForwardDuplicate for the new command elements but I'm not sure if that's a good name.

Comment 12

[Klaas Heidstra]

Attachment: patch v8 - applies on top of bug 263942

In the previous patch I was using:
> <command id="Browser:ForwardOrForwarDuplicate"
Which of course must be:
> <command id="Browser:ForwardOrForwardDuplicate"

Comment 13

[Klaas Heidstra]

For some reason the "back-forward-dropmarker" won't work in this patch, I've tried replacing Browser:Forward with Browser:ForwardOrForwardDuplicate in the code beneath but that didn't change anything.
>var backForwardDropmarker = document.getElementById("back-forward-dropmarker");
>if (backForwardDropmarker)
>  backForwardDropmarker.disabled =
>    document.getElementById('Browser:Back').hasAttribute('disabled') &&
>    document.getElementById('Browser:Forward').hasAttribute('disabled');

I was under the impression that because of: "<observes element="Browser:Forward" attribute="disabled"/>", the Browser:Forward command and the Browser:ForwardOrForwardDuplicate command "inherit" the disabled attribute from each other as soon as it changes. But apparently the disabled attribute from Browser:Forward doesn't change when the one from Browser:ForwardOrForwardDuplicate changes.

Comment 14

[Dão Gottwald [:dao]]

(In reply to comment #13)
> For some reason the "back-forward-dropmarker" won't work in this patch, I've
> tried replacing Browser:Forward with Browser:ForwardOrForwardDuplicate in the
> code beneath but that didn't change anything.

That's because removing the <observes> elements makes the unified-back-forward-button's onbroadcast attribute ineffective.

Comment 15

[Klaas Heidstra]

(In reply to comment #14)
> That's because removing the <observes> elements makes the
> unified-back-forward-button's onbroadcast attribute ineffective.
But <observes> is still in the Browser:ForwardOrForwardDuplicate <command>, so I thought the forward-button should "get" <observes> because it uses command="Browser:ForwardOrForwardDuplicate". If this is not the case, then should I just put the observes elements back in the back/forward-buttons?

Comment 16

[Dão Gottwald [:dao]]

forward-button gets the disabled attribute according to the associated command automatically. There's no extra <observes> element for this in the DOM tree; and the broadcast event is only dispatched for such elements.
One possible but unappealing solution is to handle the dropmarker explicitly wherever Browser:Back and Browser:Forward are enabled or disabled.

Comment 17

[Klaas Heidstra]

(In reply to comment #16)
> forward-button gets the disabled attribute according to the associated command
> automatically. There's no extra <observes> element for this in the DOM tree;
> and the broadcast event is only dispatched for such elements.
> One possible but unappealing solution is to handle the dropmarker explicitly
> wherever Browser:Back and Browser:Forward are enabled or disabled.

Is it a good idea to add a BackForwardDisabled <broadcaster> element to browser-sets.inc which gets disabled=true as soon as both the forward and backward commands are disabled? This could then be referenced by the dropmarker button.

Comment 18

[Dão Gottwald [:dao]]

> [...] BackForwardDisabled <broadcaster> [...] which gets disabled=true as
> soon as both the forward and backward commands are disabled

With pure markup? Not sure how that would work.

Comment 19

[Klaas Heidstra]

(In reply to comment #18)
> With pure markup? Not sure how that would work.
With some javascript ofcourse :)

Comment 20

[Klaas Heidstra]

Attachment: patch v9 - let the dropmarker observe a backForwardDisabled broadcaster

I've added a <broadcaster> which gets disabled="true" as soon as both back and forward are disabled, I then just had to add <observes element="backForwardDisabled" attribute="disabled"/> to the back-forward-dropmarker.

One problem with this poatch is that:
> +  if (backForwardDisabled == aWebNavigation.canGoBack ||
> aWebNavigation.canGoForward) {
didn't work very well so I had to use:
> +  if (aWebNavigation.canGoBack || aWebNavigation.canGoForward)
> +    var canGoBackOrForward = true;
> +  else
> +    var canGoBackOrForward = false;
> +  if (backForwardDisabled == canGoBackOrForward) {

Also I think with this patch this code in function BrowserToolboxCustomizeDone() isn't needed anymore:
>var backForwardDropmarker = document.getElementById("back-forward-dropmarker");
>if (backForwardDropmarker)
>  backForwardDropmarker.disabled =
>    document.getElementById('Browser:Back').hasAttribute('disabled') &&
>    document.getElementById('Browser:Forward').hasAttribute('disabled');

I'm not sure all this is wat you meant by comment #16, but this at least solves the problems with the back-forward-dropmarker.

Comment 21

[Dão Gottwald [:dao]]

(In reply to comment #20)
> One problem with this poatch is that:
> > +  if (backForwardDisabled == aWebNavigation.canGoBack ||
> > aWebNavigation.canGoForward) {
> didn't work very well so I had to use:
> > +  if (aWebNavigation.canGoBack || aWebNavigation.canGoForward)
> > +    var canGoBackOrForward = true;
> > +  else
> > +    var canGoBackOrForward = false;
> > +  if (backForwardDisabled == canGoBackOrForward) {

use brackets:
> if (backForwardDisabled == (aWebNavigation.canGoBack || aWebNavigation.canGoForward)) {

> Also I think with this patch this code in function
> BrowserToolboxCustomizeDone() isn't needed anymore:
> >var backForwardDropmarker = document.getElementById("back-forward-dropmarker");
> >if (backForwardDropmarker)
> >  backForwardDropmarker.disabled =
> >    document.getElementById('Browser:Back').hasAttribute('disabled') &&
> >    document.getElementById('Browser:Forward').hasAttribute('disabled');

Not sure... did you give it a try?
If you can't remove this, the broadcaster isn't useful with the dropmarker as the only observer, since you could just disable the dropmarker elsewhere directly.

Also need to take a look at gSessionHistoryObserver.

Comment 22

[Klaas Heidstra]

With this patch applied, when removing the back/forward-buttons with "customize" and then putting it back the buttons aren't synchronized with the back/forward commands, this is because of bug 309953. I've worked around this by adding this to function BrowserToolboxCustomizeDone():
> +//bug 440702: the back and forward buttons also suffer from bug 309953.
> +var backButton = document.getElementById("back-button");
> +if (backButton) {
> +  backButton.disabled =
> +    document.getElementById("Browser:Back").getAttribute("disabled") ==
> "true";
> +}
> +var forwardButton = document.getElementById("forward-button");
> +if (forwardButton) {
> +  forwardButton.disabled =
> +    document.getElementById("Browser:Forward").getAttribute("disabled") ==
> "true";
> +}

Comment 23

[Klaas Heidstra]

Attachment: patch v10 - removes unneeded code from BrowserToolboxCustomizeDone and adds a workaround for bug 309953

In this patch I've added the brackets here:
> if (backForwardDisabled == (aWebNavigation.canGoBack || aWebNavigation.canGoForward)) {

This code is removed from BrowserToolboxCustomizeDone() because I couldn't find any problems with removing it:
>var backForwardDropmarker = document.getElementById("back-forward-dropmarker");
>if (backForwardDropmarker)
>  backForwardDropmarker.disabled =
>    document.getElementById('Browser:Back').hasAttribute('disabled') &&
>    document.getElementById('Browser:Forward').hasAttribute('disabled');

And it also adds the workaround to BrowserToolboxCustomizeDone() described in comment #22.

Comment 24

[Klaas Heidstra]

(In reply to comment #23)
And it also adds this to gSessionHistoryObserver:
> +    var backFwdBroadcaster = document.getElementById("backForwardDisabled");
> +    backFwdBroadcaster.setAttribute("disabled", "true");

Comment 25

[Dão Gottwald [:dao]]

The original purpose of this bug was to simplify the code, right? (Since centralized commands are easier to maintain than widespread oncommand attributes.)
I'm afraid the current patch adds more complexity than it removes.

I think you should at least remove the broadcaster and use this instead:

> <toolbarbutton id="back-forward-dropmarker" ...
>                onbroadcast="this.disabled =
>                               document.getElementById('Browser:Back').disabled &amp;&amp;
>                               document.getElementById('Browser:Forward').disabled;">
>   ..
>   <observes element="Browser:Back" attribute="disabled"/>
>   <observes element="Browser:Forward" attribute="disabled"/>
> </toolbarbutton>

Comment 26

[Dão Gottwald [:dao]]

(In reply to comment #25)
> I think you should at least remove the broadcaster and use this instead:
> 
> > <toolbarbutton id="back-forward-dropmarker" ...
> >                onbroadcast="this.disabled =
> >                               document.getElementById('Browser:Back').disabled &amp;&amp;
> >                               document.getElementById('Browser:Forward').disabled;">
> >   ..
> >   <observes element="Browser:Back" attribute="disabled"/>
> >   <observes element="Browser:Forward" attribute="disabled"/>
> > </toolbarbutton>

command.disabled needs to be command.hasAttribute('disabled'), and it can be skipped if the element has been enabled by one of the observers:

> <toolbarbutton id="back-forward-dropmarker"
>                ...
>                onbroadcast="if (this.disabled) this.disabled =
>                               document.getElementById('Browser:Back').hasAttribute('disabled') &amp;&amp;
>                               document.getElementById('Browser:Forward').hasAttribute('disabled');">
>   <observes element="Browser:Back" attribute="disabled"/>
>   <observes element="Browser:Forward" attribute="disabled"/>
>   ...
> </toolbarbutton>

Comment 27

[Klaas Heidstra]

Attachment: patch v11 - Don't use a new broadcaster

This patch doesn't use the broadcaster anymore, I've replaced it with what you said in comment #26.

And about moving the brackets from functions to the function heads, shouldn't there be a bug about moving all those brackets in .js files?

Comment 28

[Dão Gottwald [:dao]]

Comment on attachment 327324 [details] [diff] [review]
patch v11 - Don't use a new broadcaster

>+function BrowserGoHome(aEvent) {
...
>+  var where = whereToOpenLink(aEvent, false, true);
>   var urls;
> 
>   // openUILinkIn in utilityOverlay.js doesn't handle loading multiple pages
>   switch (where) {
>   case "save":
>     urls = homePage.split("|");
>     saveURL(urls[0], null, null, true);  // only save the first page
>     break;

If you ignore Alt, the "save" case can be removed.

Comment 29

[Dão Gottwald [:dao]]

(In reply to comment #27)
> And about moving the brackets from functions to the function heads, shouldn't
> there be a bug about moving all those brackets in .js files?

Not worth a separate bug...

Comment 30

[Klaas Heidstra]

Attachment: patch v12 - Put dropmarker code for customize back

It seems like this code needs to be but back with this approach:
>var backForwardDropmarker = document.getElementById("back-forward-dropmarker");
>if (backForwardDropmarker)
>  backForwardDropmarker.disabled =
>    document.getElementById('Browser:Back').hasAttribute('disabled') &&
>    document.getElementById('Browser:Forward').hasAttribute('disabled');

Besides putting the above code back, this patch removes the "save" case you mention in comment #28.

Comment 31

[Dão Gottwald [:dao]]

Comment on attachment 327326 [details] [diff] [review]
patch v12 - Put dropmarker code for customize back

I'm not in love with Browser:FooOrFooDuplicate, but I guess all in all this is a step forward.

Comment 32

[Klaas Heidstra]

Gavin comment #53 from bug 263942 also applies to this bug, but what do you think about the rest of the patch in this bug?

Comment 33

[Dão Gottwald [:dao]]

(In reply to comment #32)
> comment #53 from bug 263942

bug 263942 comment 53

Comment 34

[Klaas Heidstra]

BrowserGoHome() has a bug when you ctrl- or middle-click click it, it doesn't make any difference if shift is pressed or not. It should open the homepage in background when shift is pressed (and the pref is set to default), but instead the new tab is selected. This is because of this code:
> // openUILinkIn in utilityOverlay.js doesn't handle loading multiple pages
> switch (where) {
> case "current":
>   loadOneOrMoreURIs(homePage);
>   break;
> case "tabshifted":
> case "tab":
>   urls = homePage.split("|");
>   var loadInBackground =
>getBoolPref("browser.tabs.loadBookmarksInBackground", false);
>   gBrowser.loadTabs(urls, loadInBackground);
>   break;
> case "window":
>   OpenBrowserWindow();
>   break;
> }

This problem can be fixed by replacing the above code with this:
> // openUILinkIn in utilityOverlay.js doesn't handle loading multiple pages
> var loadInBackground = getBoolPref("browser.tabs.loadBookmarksInBackground",
>false);
> switch (where) {
> case "current":
>   loadOneOrMoreURIs(homePage);
>   break;
> case "tabshifted":
>   loadInBackground = !loadInBackground;
>   // fall through
> case "tab":
>   urls = homePage.split("|");
>   gBrowser.loadTabs(urls, loadInBackground);
>   break;
> case "window":
>   OpenBrowserWindow();
>   break;
> }

Should I file a new bug for this problem or fix it within this bug?

Comment 35

[Dão Gottwald [:dao]]

New bug.

Comment 36

[Klaas Heidstra]

(In reply to comment #35)
> New bug.

I filed bug 444911
I think the easiest solution is to fix it within this bug and make bug 444911 depend on this one, is that a good idea?

Comment 37

[Dão Gottwald [:dao]]

(In reply to comment #36)
> I filed bug 444911
> I think the easiest solution is to fix it within this bug and make bug 444911
> depend on this one, is that a good idea?

Seems like an unrelated bug (if it's not intentional). The more changes you mix into this bug, the sooner you'll get the ui-review denied ;)

Comment 39

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 327326 [details] [diff] [review]
patch v12 - Put dropmarker code for customize back

I might regret this later, but after wrestling with it I don't think there's a lot of cost to adding this option. It's pretty irregular for a context menu to react to a middle click, which means that it's not going to be discoverable, but also means that we're not going to break any other expectation of the functionality.

If we do this, Ctrl/Cmd + click on these actions should also open new tabs.

Comment 40

[Klaas Heidstra]

(In reply to comment #39)
> If we do this, Ctrl/Cmd + click on these actions should also open new tabs.

That is indeed the case for the patch in this bug

Comment 41

[:Gavin Sharp [email: [e-mail]]]

Oh, one thing I forgot to mention: it seems like it would be better to take the wallpaper patch in bug 309953 rather than extending the hack from bug 287105. I'll comment there.

Comment 42

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/f8554e4a7809