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)
Firefox
Toolbars and Customization
Tracking
()
RESOLVED
FIXED
Firefox 28
People
(Reporter: mconley, Assigned: bwinton)
References
Details
Attachments
(1 file, 2 obsolete files)
5.99 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
I'm already starting to work on this, so I might as well make it official…
Assignee: nobody → bwinton
Assignee | ||
Comment 2•12 years ago
|
||
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)
Comment 3•12 years ago
|
||
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
Assignee | ||
Comment 4•12 years ago
|
||
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…
Comment 5•12 years ago
|
||
Yea, I'm digging now. WFM as expected on OSX.
Comment 6•12 years ago
|
||
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...
Assignee | ||
Comment 7•12 years ago
|
||
(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!
Comment 8•12 years ago
|
||
(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")
Assignee | ||
Comment 9•12 years ago
|
||
(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!
Comment 10•12 years ago
|
||
(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.
Assignee | ||
Comment 11•12 years ago
|
||
Alright, I'll fix that one too, while I'm at it, then.
(The next patch will go up once it gets an r±.)
Reporter | ||
Comment 12•12 years ago
|
||
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?
Assignee | ||
Comment 13•12 years ago
|
||
No, but I think Unfocused is looking into it…
Reporter | ||
Comment 14•12 years ago
|
||
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.
Reporter | ||
Comment 15•12 years ago
|
||
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+
Reporter | ||
Comment 16•12 years ago
|
||
I ui-review?'d shorlander, but bwinton, I'm open to your input on this as well.
Reporter | ||
Comment 17•12 years ago
|
||
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+
Reporter | ||
Comment 18•12 years ago
|
||
Landed on jamun as https://hg.mozilla.org/projects/jamun/rev/37168ecb8b05
Whiteboard: [fixed in jamun]
Reporter | ||
Comment 19•12 years ago
|
||
Landed on UX as https://hg.mozilla.org/projects/ux/rev/37168ecb8b05
Whiteboard: [fixed in jamun] → [fixed in jamun][fixed in ux]
Comment 20•11 years ago
|
||
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.
Description
•