720431 - Can't close Style Editor with cmd+w

Status: VERIFIED FIXED

(DevTools :: Style Editor, defect, P2)

Description

[Josh Matthews [:jdm]]

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]]

Attachment: patch v1

Comment 2

[Cedric Vivier [:cedricv]]

We should nominate this for aurora approval after bake time in m-c.

Comment 3

[Paul Rouget [:paul]]

Attachment: patch v1 - rebased

Comment 4

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/7b66794634ec

Comment 5

[Paul Rouget [:paul]]

I will a? this patch once in central.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/7b66794634ec

Comment 8

[Rob Campbell [:rc] (:robcee)]

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]]

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)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a04e7209a49c

Comment 11

[Francesco Lodolo [:flod]]

Aurora is supposed to be string frozen...

Comment 12

[Axel Hecht [:Pike]]

Alex, you approved a string-change bug here for aurora.

Can we get this backed out?

Comment 13

[Alex Keybl [:akeybl]]

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)]

(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]]

(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]]

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)]

(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]]

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)]

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]]

> 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]]

(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]]

I'll do a post in .l10n, and one in .planning.

Comment 23

[Mihaela Velimiroviciu (:mihaelav)]

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)]

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)