876816 - Defect - Flyouts should have standard widths

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Flyouts, defect, P3)

Description

[Jim Mathies [:jimm]]

Per comments in bug 831955, we need to make sure our flyouts use one of two widths - narrow (346 pixels) or wide (646 pixels).

Comment 1

[Brian R. Bondy [:bbondy]]

If we do this and it causes any extra work, we should backout instead of posting new defects for v1.

Comment 2

[Brian R. Bondy [:bbondy]]

p=1

Comment 3

[Kamil Jozwiak [:kjozwiak]]

I will give this a shot!

Comment 4

[Kamil Jozwiak [:kjozwiak]]

Attachment: fixed the widths for prefs/about panels

Changed the widths for both prefs & about panels to 346 as per comment 0. Ally, looks like you have fixed the width for the sync panel in bug 845468.

Made sure that all the panel widths matched in Firefox Metro, also ensured that nothing was being cut off for "Options", "Sync" & "About". Went through some basic functionality and ensured everything was still working correctly.

Comment 5

[Allison Naaktgeboren :ally]

Comment on attachment 777545 [details] [diff] [review]
fixed the widths for prefs/about panels

Review of attachment 777545 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! This patch is correct as is, but I think we can make it even better. Since we want all the flyouts to be the same width (and probably some other attributes), why don't we seet a class in the xul on the flyouts so we can use css class selectors? That way we can control the style of the flyouts in one place and ensure they always match. For an example, the "meta-section" class used here http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser.xul#206 (you can see it used several places in the start page here) matches to http://mxr.mozilla.org/mozilla-central/source/browser/metro/theme/platform.css#657

So in browser.xul change the flyouts like 
<flyoutpanel id="prefs-flyoutpanel" headertext="&optionsHeader.title;"> 
to add something like
<flyoutpanel id="prefs-flyoutpanel" class="prefFlyouts" headertext="&optionsHeader.title;">

add something like to browser.css
.prefsFlyouts {
  // common rules in here like the width
}

Also please remove the old rules you've just consolidated from their id selectors[2] (like #about-flyoutpanel) as those have higher precedence [3] so they would override your new class rule. :)

Please let us know if you have questions or get stuck. We're happy to help.

[1] https://developer.mozilla.org/en-US/docs/Web/CSS/Class_selectors
[2] https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Getting_started/Selectors
[3] "CSS gives priority to the rule that has the more specific selector. An ID selector is more specific than a class selector, which in turn is more specific than a tag selector." from [2]

Comment 6

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

drive-by nit: Don't call the class "prefFlyouts" since that would be specific to just the "preferences" flyout.  Something like "flyout" or "flyoutPanel" would be more appropriate.

Also, it might be best to put the CSS for flyouts in flyoutpanel.css (maybe we can rename that to just "flyout.css"... but I digress)

Comment 7

[Brian R. Bondy [:bbondy]]

Good idea re the class. Maybe call it narrowFlyout and we can later use wideFlyout if we need a wider one.

Comment 8

[Kamil Jozwiak [:kjozwiak]]

Thanks for the feedback! Will give it another shot tomorrow.

Comment 9

[Kamil Jozwiak [:kjozwiak]]

Attachment: fixed the widths for Prefs/Sync/About panels v2

Created the narrowFlyout class and then added them into the Pref, Sync and About panels.

Went through all three flyouts and made sure everything was still working, the correct widths are being used and nothing is being cut off.

Thanks for the help and review Ally!

Comment 10

[Allison Naaktgeboren :ally]

Comment on attachment 778781 [details] [diff] [review]
fixed the widths for Prefs/Sync/About panels v2

Review of attachment 778781 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there! Looks good & mbrubeck was nice enough to test it while my trees build this morning.

Tim is right that the new css rule should move to the flyoutpanels.css file. While the old rules did live in browser.css, now that we have a separate css file dedicated to flyouts, we should put all our css related to flyouts in there. The good news is that it should be a quick & easy change.

Comment 11

[Kamil Jozwiak [:kjozwiak]]

Sound good! I will change this on Friday when I get home from my work trip (currently traveling and don't have access to my environment PC as its in  Toronto!

Comment 12

[Allison Naaktgeboren :ally]

Comment on attachment 778781 [details] [diff] [review]
fixed the widths for Prefs/Sync/About panels v2

look forward to seeing it. :)

Comment 13

[Asa Dotzler [:asa]]

Can one of you all take a look at bug 898457 and see if that's related or could be fixed here?

Comment 14

[Brian R. Bondy [:bbondy]]

Looks like an unrelated issue, I think it's due to localized strings not wrapping in the about panel.  Should be looked at independently to this bug.

Comment 15

[Kamil Jozwiak [:kjozwiak]]

Attachment: fixed the widths for Prefs/Sync/About panels v3

Ally, added the following changes:

- removed the class from browser.css and added it into flyoutpanels.css
- renamed the class to flyout-narrow to match the naming convention in flyoutpanels.css
- changed the names of the class in browser.xul to match the new name in flyoutpanels.css

Went through all three flyout panels (Pref, About, Sync) and made sure they where working without any issues and used the same widths.

Hopefully everything looks good this time! If not, just let me know if anything else needs to be changed.

Comment 16

[Allison Naaktgeboren :ally]

Comment on attachment 782395 [details] [diff] [review]
fixed the widths for Prefs/Sync/About panels v3

Review of attachment 782395 [details] [diff] [review]:
-----------------------------------------------------------------

\o/ Let's Ship it!

How about bug 885383 next? It's a little meatier.

Comment 17

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/14724efcf7f7

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/14724efcf7f7

Comment 19

[Josh Tumath]

Attachment: Screenshot of the bug

Comment 20

[Josh Tumath]

Comment on attachment 784057 [details]
Screenshot of the bug

Whoops. I posted this screenshot in the wrong bug. Apologies.

Comment 21

[Manuela Muntean [Away]]

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130819030205

WFM
Tested on windows 8 64bit using latest Nightly for iteration #12. Flyouts seem to all have same widths.

This should also be verified on a device.

Comment 22

[Samvedana (:Samvedana)]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130826074752
Built from http://hg.mozilla.org/mozilla-central/rev/14b1e8c2957e

WFM
Tested on windows 8 using latest nightly for iteration-12. All fly-out width was same.