Closed Bug 858662 Opened 12 years ago Closed 11 years ago

Make clicking on the blue grid in customization mode exit customization mode

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: mconley, Assigned: bwinton)

References

Details

Attachments

(1 file, 2 obsolete files)

UX wants lots of escape hatches for customization mode. Clicking on the blue grid is one of them. UX has requested that we try to slip this into M2.
Blocks: 858660
I'm already starting to work on this, so I might as well make it official…
Assignee: nobody → bwinton
Attached patch The first cut at this patch. (obsolete) — Splinter Review
This is all fairly straight forward. One small bit of cleanup I did was to change the "==" to "===" in the click handler, to avoid silly type-conversion mismatches. One thing this doesn't handle is if we click and drag on the blueprint to move the window. Or, rather, that still registers as a click, and so we get out of customize mode. It seems a little complicated to handle that to me, so I suggest we land it in a follow-up patch. Thanks, Blake.
Attachment #734000 - Flags: ui-review?(mconley)
Attachment #734000 - Flags: review?(mconley)
Hmm, this doesn't seem to work for me on Windows 8. Except via double-clicking, which also maximizes/restores the window. I assume you're on OSX?
Status: NEW → ASSIGNED
Yeah. I don't have a Windows box handy for the next week, though. Could you (or Mike) dig into it a little? I'll test it out on Linux, probably tomorrow-ish…
Yea, I'm digging now. WFM as expected on OSX.
Comment on attachment 734000 [details] [diff] [review] The first cut at this patch. > case "click": > if (aEvent.button == 0 && >- aEvent.originalTarget == this.window.PanelUI.menuButton) { >+ (aEvent.originalTarget === this.window.PanelUI.menuButton) || >+ (aEvent.originalTarget === this.document.getElementById("tab-view-deck"))) { I fail to see how === is any better than == here. Also, there are some unnecessary parentheses...
(In reply to Dão Gottwald [:dao] from comment #6) > I fail to see how === is any better than == here. If they're the same, then why not use the safer one everywhere? > Also, there are some unnecessary parentheses... Fixed! (Well, I removed a pair of the parentheses. I'm not entirely sure whether I can remove the ones around the ||, and since I'm not sure, I think it's better to leave them in to indicate the intent of the code. :) Thanks!
(In reply to Blake Winton (:bwinton) from comment #7) > (In reply to Dão Gottwald [:dao] from comment #6) > > I fail to see how === is any better than == here. > > If they're the same, then why not use the safer one everywhere? The short answer is that we have no such convention, so this kind of "cleanup" just introduces inconsistency. I also don't think I buy the safety argument. == just isn't strict about types, which doesn't matter most of the time (like here) and can actually be desirable behavior in other cases. I can't remember having come across a bug that was caused by == being used instead of ===. > > Also, there are some unnecessary parentheses... > > Fixed! (Well, I removed a pair of the parentheses. I'm not entirely sure > whether I can remove the ones around the ||, The outer parentheses are needed since || is weaker than &&. (try "0 && (0 || 1)" vs. "0 && 0 || 1")
(In reply to Dão Gottwald [:dao] from comment #8) > (In reply to Blake Winton (:bwinton) from comment #7) > > (In reply to Dão Gottwald [:dao] from comment #6) > > > I fail to see how === is any better than == here. > > > > If they're the same, then why not use the safer one everywhere? > > The short answer is that we have no such convention, so this kind of > "cleanup" just introduces inconsistency. The other comparison in this function uses "===". If anything, changing these to "===" would reduce inconsistency. :) > > > Also, there are some unnecessary parentheses... > > Fixed! (Well, I removed a pair of the parentheses. I'm not entirely sure > > whether I can remove the ones around the ||, > The outer parentheses are needed since || is weaker than &&. (try "0 && (0 > || 1)" vs. "0 && 0 || 1") Nice, I also realized that I was missing a set of parentheses in my original patch, so I've added them in the right place instead of where they were. Thanks again for pointing that out!
(In reply to Blake Winton (:bwinton) from comment #9) > The other comparison in this function uses "===". If anything, changing > these to "===" would reduce inconsistency. :) No, it adds to it. The existing (also unnecessary) one just isn't in line with most of our code base.
Alright, I'll fix that one too, while I'm at it, then. (The next patch will go up once it gets an r±.)
At least on Windows, this requires me to double-click on the grid before it triggers the click event and sends us out of customization mode. Any idea why?
No, but I think Unfocused is looking into it…
Ok, I figured it out - it's because there's a windowdragbox binding on the deck when in customization mode. That's capturing the click event. Removing that binding means that clicking and dragging the grid no longer allows us to move the window, however, it does mean that an immediate click causes us to exit customization mode.
Attached patch Patch v2 (obsolete) — Splinter Review
Removes the windowdragbox binding, addresses Dao's concerns, and reduces some duplication. Assuming UX is happy with the inability to drag with the grid, I'm happy with this.
Attachment #734000 - Attachment is obsolete: true
Attachment #734000 - Flags: ui-review?(mconley)
Attachment #734000 - Flags: review?(mconley)
Attachment #735337 - Flags: ui-review?(shorlander)
Attachment #735337 - Flags: review+
I ui-review?'d shorlander, but bwinton, I'm open to your input on this as well.
Attached patch Patch v2.1Splinter Review
De-bitrotting for latest trunk. I don't have a whole lot of faith in hearing from anyone from the UX team on this, so I'm going to ui-r+ this, and we can back it out if they disagree.
Attachment #735337 - Attachment is obsolete: true
Attachment #735337 - Flags: ui-review?(shorlander)
Attachment #735940 - Flags: review+
Whiteboard: [fixed in jamun]
Whiteboard: [fixed in jamun] → [fixed in jamun][fixed in ux]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in jamun][fixed in ux]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: