Last Comment Bug 720431 - Can't close Style Editor with cmd+w
: Can't close Style Editor with cmd+w
Status: VERIFIED FIXED
[styleeditor][qa!]
: verified-beta
Product: Firefox
Classification: Client Software
Component: Developer Tools: Style Editor (show other bugs)
: unspecified
: x86 Mac OS X
: P2 normal (vote)
: Firefox 12
Assigned To: Cedric Vivier [:cedricv]
:
Mentors:
: 715304 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-23 10:59 PST by Josh Matthews [:jdm] (away until 9/3)
Modified: 2012-03-21 07:30 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
verified


Attachments
patch v1 (2.76 KB, patch)
2012-01-25 02:45 PST, Cedric Vivier [:cedricv]
paul: review+
Details | Diff | Splinter Review
patch v1 - rebased (2.50 KB, patch)
2012-01-25 05:02 PST, Paul Rouget [:paul]
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Josh Matthews [:jdm] (away until 9/3) 2012-01-23 10:59:33 PST
The Style Editor acts like a separate window, similar to the Download Manager, but I cannot close it with the regular window-closing keyboard shortcut.
Comment 1 Cedric Vivier [:cedricv] 2012-01-25 02:45:42 PST
Created attachment 591398 [details] [diff] [review]
patch v1
Comment 2 Cedric Vivier [:cedricv] 2012-01-25 03:37:36 PST
We should nominate this for aurora approval after bake time in m-c.
Comment 3 Paul Rouget [:paul] 2012-01-25 05:02:19 PST
Created attachment 591427 [details] [diff] [review]
patch v1 - rebased
Comment 4 Paul Rouget [:paul] 2012-01-25 05:13:08 PST
https://hg.mozilla.org/integration/fx-team/rev/7b66794634ec
Comment 5 Paul Rouget [:paul] 2012-01-25 05:36:44 PST
I will a? this patch once in central.
Comment 6 Rob Campbell [:rc] (:robcee) 2012-01-25 08:31:35 PST
*** Bug 715304 has been marked as a duplicate of this bug. ***
Comment 7 Tim Taubert [:ttaubert] 2012-01-25 09:08:26 PST
https://hg.mozilla.org/mozilla-central/rev/7b66794634ec
Comment 8 Rob Campbell [:rc] (:robcee) 2012-01-25 10:25:56 PST
Comment on attachment 591427 [details] [diff] [review]
patch v1 - rebased

[Approval Request Comment]
Regression caused by (bug #): Not a regression, new feature.
User impact if declined: User won't be able to close Style Editor window with common shortcut.
Testing completed (on m-c, etc.):  on m-c, local testing.
Risk to taking this patch (and alternatives if risky): Not risky. Defines simple xul keyboard shortcut for closing the window.
Comment 9 Alex Keybl [:akeybl] 2012-01-25 18:31:01 PST
Comment on attachment 591427 [details] [diff] [review]
patch v1 - rebased

[Triage Comment]
Regression in new feature, approved for Aurora.
Comment 10 Rob Campbell [:rc] (:robcee) 2012-01-26 12:48:34 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/a04e7209a49c
Comment 11 Francesco Lodolo [:flod] 2012-01-27 11:14:22 PST
Aurora is supposed to be string frozen...
Comment 12 Axel Hecht [:Pike] 2012-01-27 11:35:40 PST
Alex, you approved a string-change bug here for aurora.

Can we get this backed out?
Comment 13 Alex Keybl [:akeybl] 2012-01-27 13:02:18 PST
Didn't catch the fact that there was a string change here - only saw the shortcut addition. Yes, we can back this out, or possibly remove the string if the devtools guys are comfortable with that change (not sure where the string shows up in the UI).

Rob - can you take next action on this?
Comment 14 Rob Campbell [:rc] (:robcee) 2012-01-27 13:09:23 PST
(In reply to Alex Keybl [:akeybl] from comment #13)
> Didn't catch the fact that there was a string change here - only saw the
> shortcut addition. Yes, we can back this out, or possibly remove the string
> if the devtools guys are comfortable with that change (not sure where the
> string shows up in the UI).
> 
> Rob - can you take next action on this?

So we're ok with not having a keyboard shortcut on beta (after the merge)? I can back out if that's the call, but I'd really prefer to have this in.
Comment 15 Alex Keybl [:akeybl] 2012-01-27 13:20:52 PST
(In reply to Rob Campbell [:rc] (robcee) from comment #14)
> So we're ok with not having a keyboard shortcut on beta (after the merge)? I
> can back out if that's the call, but I'd really prefer to have this in.

Our options are

1) Back out the change
2) Figure out a way to get this feature in without a string
3) Try to find another option for localizing the feature with Axel

If you'd like to pursue #3, please start an email thread with us to discuss further.
Comment 16 Axel Hecht [:Pike] 2012-01-27 13:44:10 PST
Given that this is a command key, l10n-merge could do the right thing.

I'm concerned about the amount of string-freeze bustages in this cycle, at large, though.
Comment 17 Dave Camp (:dcamp) 2012-01-27 16:06:52 PST
(In reply to Axel Hecht [:Pike] from comment #16)
> Given that this is a command key, l10n-merge could do the right thing.

Sorry, I don't know our l10n infrastructure as well as I should - is this a suggestion for #3 (another option for localizing)?  How much trouble is that, can we help make it easier?
Comment 18 Axel Hecht [:Pike] 2012-01-27 16:11:04 PST
l10n-merge is the jargon for our build-time fallback to en-US for missing strings in localizations. So with the status-quo, we're falling back to crtl-w if there's no localization, which is probably right anyway, as control keys should not be localized commonly.

That doesn't make things good, it just makes them OK to stay as is. Bugs like this should be found before landing on central, or on central. If it's not baked enough on central that it needs string fixes to not suck, switch it off.
Comment 19 Rob Campbell [:rc] (:robcee) 2012-01-28 09:36:48 PST
Alright then. Based on this it sounds like we might be able to get away with it. It was kind of a stupid omission to not get that in before aurora, and now here we are at the end of the cycle... Balls were dropped then rolled along the floor.

Axel, let us know if this causes any problems or if you need any other action. Would landing an extra l10n comment to the file be more helpful or worse at this point?
Comment 20 Francesco Lodolo [:flod] 2012-01-28 21:52:57 PST
> Based on this it sounds like we might be able to get away with it.

https://l10n-stage-sj.mozilla.org/shipping/dashboard?tree=fx_aurora

IMO situation is not good thanks to the last two bugs that broke string freeze.

Honestly, right now I don't understand if I have to add the missing string or wait for this change to be backed out and my locale to turn back green. Please tell us what to do (i.e. what you plan to do), since we're just a couple of days from the end of this cycle!
Comment 21 Alex Keybl [:akeybl] 2012-01-29 20:15:23 PST
(In reply to Francesco Lodolo [:flod] from comment #20)
> > Based on this it sounds like we might be able to get away with it.
> 
> https://l10n-stage-sj.mozilla.org/shipping/dashboard?tree=fx_aurora
> 
> IMO situation is not good thanks to the last two bugs that broke string
> freeze.
> 
> Honestly, right now I don't understand if I have to add the missing string
> or wait for this change to be backed out and my locale to turn back green.
> Please tell us what to do (i.e. what you plan to do), since we're just a
> couple of days from the end of this cycle!

I don't know enough about l10n to fully advise but let me put it this way - if it's normal to land a string at this point on Aurora and it doesn't carry risk, feel free. Otherwise what Axel's saying is that the keyboard shortcut will suffice without needing localizations.
Comment 22 Axel Hecht [:Pike] 2012-01-29 23:55:30 PST
I'll do a post in .l10n, and one in .planning.
Comment 23 Mihaela Velimiroviciu (:mihaelav) 2012-02-07 05:42:54 PST
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:11.0) Gecko/20100101 Firefox/11.0

Verified the change on the Firefox 11 beta 1: CMD(Windows button)+w closes the style editor window.
Comment 24 Mihaela Velimiroviciu (:mihaelav) 2012-03-21 07:30:42 PDT
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:12.0) Gecko/20100101 Firefox/12.0
Verified the fix on latest Firefox beta (12.0b1)

Note You need to log in before you can comment on or make changes to this bug.