Closed
Bug 440702
Opened 13 years ago
Closed 13 years ago
Link modifiers should use the command attribute instead of oncommand(and remove observes elements)
Categories
(Firefox :: General, defect)
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.
Assignee | ||
Comment 1•13 years ago
|
||
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)
Assignee | ||
Updated•13 years ago
|
Whiteboard: [263942 must land first] [has patch] [needs review dao]
Assignee: nobody → klaas1988
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Updated•13 years ago
|
Blocks: link-modifiers
Assignee | ||
Updated•13 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•13 years ago
|
||
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)
Assignee | ||
Comment 3•13 years ago
|
||
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)
Assignee | ||
Updated•13 years ago
|
Attachment #326137 -
Attachment description: 326053: patch v3 - applies on top of bug 263942 → patch v3 - applies on top of bug 263942
Assignee | ||
Comment 4•13 years ago
|
||
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)
Assignee | ||
Comment 5•13 years ago
|
||
Update again.
Attachment #326165 -
Attachment is obsolete: true
Attachment #326169 -
Flags: review?(dao)
Attachment #326165 -
Flags: review?(dao)
Assignee | ||
Comment 6•13 years ago
|
||
Update.
Attachment #326169 -
Attachment is obsolete: true
Attachment #326169 -
Flags: review?(dao)
Comment 7•13 years ago
|
||
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.
Assignee | ||
Comment 8•13 years ago
|
||
(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•13 years ago
|
||
(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•13 years ago
|
||
As I mentioned in bug 263942, I think supporting middle-click of menuitems is undesirable.
Assignee | ||
Comment 11•13 years ago
|
||
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)
Assignee | ||
Comment 12•13 years ago
|
||
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)
Assignee | ||
Comment 13•13 years ago
|
||
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•13 years ago
|
||
(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.
Assignee | ||
Comment 15•13 years ago
|
||
(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•13 years ago
|
||
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.
Assignee | ||
Comment 17•13 years ago
|
||
(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•13 years ago
|
||
> [...] 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.
Assignee | ||
Comment 19•13 years ago
|
||
(In reply to comment #18) > With pure markup? Not sure how that would work. With some javascript ofcourse :)
Assignee | ||
Comment 20•13 years ago
|
||
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)
Comment 21•13 years ago
|
||
(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.
Assignee | ||
Comment 22•13 years ago
|
||
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"; > +}
Assignee | ||
Comment 23•13 years ago
|
||
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)
Assignee | ||
Comment 24•13 years ago
|
||
(In reply to comment #23) And it also adds this to gSessionHistoryObserver: > + var backFwdBroadcaster = document.getElementById("backForwardDisabled"); > + backFwdBroadcaster.setAttribute("disabled", "true");
Comment 25•13 years ago
|
||
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 &&
> document.getElementById('Browser:Forward').disabled;">
> ..
> <observes element="Browser:Back" attribute="disabled"/>
> <observes element="Browser:Forward" attribute="disabled"/>
> </toolbarbutton>
Comment 26•13 years ago
|
||
(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 && > > 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') && > document.getElementById('Browser:Forward').hasAttribute('disabled');"> > <observes element="Browser:Back" attribute="disabled"/> > <observes element="Browser:Forward" attribute="disabled"/> > ... > </toolbarbutton>
Assignee | ||
Comment 27•13 years ago
|
||
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 28•13 years ago
|
||
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-
Comment 29•13 years ago
|
||
(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...
Assignee | ||
Comment 30•13 years ago
|
||
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 31•13 years ago
|
||
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+
Assignee | ||
Updated•13 years ago
|
Whiteboard: [263942 must land first] [has patch] [needs review dao] → [263942 must land first] [has patch] [needs review gavin.sharp]
Assignee | ||
Comment 32•13 years ago
|
||
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•13 years ago
|
||
(In reply to comment #32) > comment #53 from bug 263942 bug 263942 comment 53
Assignee | ||
Comment 34•13 years ago
|
||
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•13 years ago
|
||
New bug.
Assignee | ||
Comment 36•13 years ago
|
||
(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•13 years ago
|
||
(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•13 years ago
|
||
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+
Assignee | ||
Comment 40•13 years ago
|
||
(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
Updated•13 years ago
|
Attachment #327326 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Whiteboard: [263942 must land first] [has patch] [needs review gavin.sharp] → [263942 must land first] [has patch] [has review]
Comment 41•13 years ago
|
||
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•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/f8554e4a7809
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [263942 must land first] [has patch] [has review]
Target Milestone: --- → Firefox 3.1b1
You need to log in
before you can comment on or make changes to this bug.
Description
•