The default bug view has changed. See this FAQ.

Defect - Flyouts should have standard widths

VERIFIED FIXED in Firefox 25

Status

Firefox for Metro
Flyouts
P3
normal
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: jimm, Assigned: kjozwiak)

Tracking

({polish})

Trunk
Firefox 25
x86_64
Windows 8.1
polish
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=1)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

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

Updated

4 years ago
Blocks: 859003
No longer blocks: 841214
Priority: -- → P3
Whiteboard: feature=defect c=tbd u=tbd p=0
If we do this and it causes any extra work, we should backout instead of posting new defects for v1.
p=1

Updated

4 years ago
Whiteboard: feature=defect c=tbd u=tbd p=0 → feature=defect c=Settings_pane_options_and_about u=tbd p=0

Updated

4 years ago
Blocks: 831955, 831958
Whiteboard: feature=defect c=Settings_pane_options_and_about u=tbd p=0 → feature=defect c= u=tbd p=0

Updated

4 years ago
Whiteboard: feature=defect c= u=tbd p=0 → feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=0

Updated

4 years ago
Summary: Flyouts should have standard widths → Defect - Flyouts should have standard widths
(Assignee)

Comment 3

4 years ago
I will give this a shot!
Assignee: nobody → kamiljoz
(Assignee)

Comment 4

4 years ago
Created attachment 777545 [details] [diff] [review]
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.
Attachment #777545 - Flags: review?(ally)

Updated

4 years ago
Blocks: 886790
No longer blocks: 859003
Status: NEW → ASSIGNED
QA Contact: jbecerra
Whiteboard: feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=0 → feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=1
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]
Attachment #777545 - Flags: review?(ally) → feedback+
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)
Good idea re the class. Maybe call it narrowFlyout and we can later use wideFlyout if we need a wider one.
(Assignee)

Comment 8

4 years ago
Thanks for the feedback! Will give it another shot tomorrow.
(Assignee)

Comment 9

4 years ago
Created attachment 778781 [details] [diff] [review]
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!
Attachment #778781 - Flags: review?(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.
Attachment #778781 - Flags: review?(ally)
Attachment #778781 - Flags: review+
Attachment #778781 - Flags: feedback+
(Assignee)

Comment 11

4 years ago
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 on attachment 778781 [details] [diff] [review]
fixed the widths for Prefs/Sync/About panels v2

look forward to seeing it. :)
Attachment #778781 - Flags: review+

Updated

4 years ago
Blocks: 893311
No longer blocks: 886790

Comment 13

4 years ago
Can one of you all take a look at bug 898457 and see if that's related or could be fixed here?
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.
(Assignee)

Comment 15

4 years ago
Created attachment 782395 [details] [diff] [review]
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.
Attachment #777545 - Attachment is obsolete: true
Attachment #778781 - Attachment is obsolete: true
Attachment #782395 - Flags: review?(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.
Attachment #782395 - Flags: review?(ally) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/14724efcf7f7
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/14724efcf7f7
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25

Updated

4 years ago
Depends on: 900239

Comment 19

4 years ago
Created attachment 784057 [details]
Screenshot of the bug

Comment 20

4 years ago
Comment on attachment 784057 [details]
Screenshot of the bug

Whoops. I posted this screenshot in the wrong bug. Apologies.
Attachment #784057 - Attachment is obsolete: true

Updated

4 years ago
No longer depends on: 900239
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.
Status: RESOLVED → VERIFIED
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.
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.