Closed Bug 440702 Opened 11 years ago Closed 11 years ago

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

Categories

(Firefox :: General, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 3.1b1

People

(Reporter: klaas1988, Assigned: klaas1988)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file, 11 obsolete files)

13.45 KB, patch
dao
: review+
Gavin
: review+
beltzner
: ui-review+
Details | Diff | Splinter Review
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.
Depends on: 263942
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.
Attachment #325986 - Flags: review?(dao)
Whiteboard: [263942 must land first] [has patch] [needs review dao]
Assignee: nobody → klaas1988
Status: UNCONFIRMED → NEW
Ever confirmed: true
Status: NEW → ASSIGNED
In the previous patch I have forgotten to replace one oncommand attribute with command, that is corrected in this patch.
Attachment #325986 - Attachment is obsolete: true
Attachment #326053 - Flags: review?(dao)
Attachment #325986 - Flags: review?(dao)
Blocks: 422732
I set ignoreAlt to always be true for browserGoHome to so that it fixes bug 422732 completely now.
Attachment #326053 - Attachment is obsolete: true
Attachment #326137 - Flags: review?(dao)
Attachment #326053 - Flags: review?(dao)
Attachment #326137 - Attachment description: 326053: patch v3 - applies on top of bug 263942 → patch v3 - applies on top of bug 263942
Update so that it applies to the latest patch in bug 263942.
Attachment #326137 - Attachment is obsolete: true
Attachment #326165 - Flags: review?(dao)
Attachment #326137 - Flags: review?(dao)
Update again.
Attachment #326165 - Attachment is obsolete: true
Attachment #326169 - Flags: review?(dao)
Attachment #326165 - Flags: review?(dao)
Update.
Attachment #326169 - Attachment is obsolete: true
Attachment #326169 - Flags: review?(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.
(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?
(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.
As I mentioned in bug 263942, I think supporting middle-click of menuitems is undesirable.
I'm now using BackOrBackDuplicate and ForwardOrForwardDuplicate for the new command elements but I'm not sure if that's a good name.
Attachment #326178 - Attachment is obsolete: true
Attachment #326466 - Flags: ui-review?(beltzner)
Attachment #326466 - Flags: review?(dao)
In the previous patch I was using:
> <command id="Browser:ForwardOrForwarDuplicate"
Which of course must be:
> <command id="Browser:ForwardOrForwardDuplicate"
Attachment #326466 - Attachment is obsolete: true
Attachment #326493 - Flags: ui-review?(beltzner)
Attachment #326493 - Flags: review?(dao)
Attachment #326466 - Flags: ui-review?(beltzner)
Attachment #326466 - Flags: review?(dao)
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.
(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.
(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?
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.
(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.
> [...] 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.
(In reply to comment #18)
> With pure markup? Not sure how that would work.
With some javascript ofcourse :)

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.
Attachment #326493 - Attachment is obsolete: true
Attachment #326811 - Flags: ui-review?(beltzner)
Attachment #326811 - Flags: review?(dao)
Attachment #326493 - Flags: ui-review?(beltzner)
Attachment #326493 - Flags: review?(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.
Depends on: 309953
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";
> +}
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.
Attachment #326811 - Attachment is obsolete: true
Attachment #326914 - Flags: ui-review?(beltzner)
Attachment #326914 - Flags: review?(dao)
Attachment #326811 - Flags: ui-review?(beltzner)
Attachment #326811 - Flags: review?(dao)
(In reply to comment #23)
And it also adds this to gSessionHistoryObserver:
> +    var backFwdBroadcaster = document.getElementById("backForwardDisabled");
> +    backFwdBroadcaster.setAttribute("disabled", "true");

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>
(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>
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?
Attachment #326914 - Attachment is obsolete: true
Attachment #327324 - Flags: ui-review?(beltzner)
Attachment #327324 - Flags: review?(dao)
Attachment #326914 - Flags: ui-review?(beltzner)
Attachment #326914 - Flags: review?(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.
Attachment #327324 - Flags: review?(dao) → review-
(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...
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.
Attachment #327324 - Attachment is obsolete: true
Attachment #327326 - Flags: ui-review?(beltzner)
Attachment #327326 - Flags: review?(dao)
Attachment #327324 - Flags: ui-review?(beltzner)
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.
Attachment #327326 - Flags: review?(gavin.sharp)
Attachment #327326 - Flags: review?(dao)
Attachment #327326 - Flags: review+
Whiteboard: [263942 must land first] [has patch] [needs review dao] → [263942 must land first] [has patch] [needs review gavin.sharp]
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?
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?
New bug.
(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?

(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 ;)
Blocks: 448546
Duplicate of this bug: 448556
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.
Attachment #327326 - Flags: ui-review?(beltzner) → ui-review+
(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
Attachment #327326 - Flags: review?(gavin.sharp) → review+
Keywords: checkin-needed
Whiteboard: [263942 must land first] [has patch] [needs review gavin.sharp] → [263942 must land first] [has patch] [has review]
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.
http://hg.mozilla.org/mozilla-central/rev/f8554e4a7809
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [263942 must land first] [has patch] [has review]
Target Milestone: --- → Firefox 3.1b1
No longer blocks: 448546
Depends on: 483919
You need to log in before you can comment on or make changes to this bug.