Closed Bug 1386514 Opened 7 years ago Closed 7 years ago

Support RWD for about:preferences to match Photon visual spec

Categories

(Firefox :: Settings UI, enhancement, P1)

Product:
Firefox
Component:
Settings UI
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- verified

People

(Reporter: rickychien, Assigned: evanxd)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-preference])

Attachments

(3 files)

Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px.
(deleted), text/x-review-board-request
jaws
: review+
Details
screenshot.png
(deleted), image/png
Details
screenshot.png showing category width
(deleted), image/png
Details 
Visual spec https://mozilla.invisionapp.com/share/X8BGCX9PD#/screens/244673481
Flags: qe-verify+
Assignee: nobody → evan
Status: NEW → ASSIGNED
Priority: P2 → P1
Whiteboard: [photon-preference][triage] → [photon-preference]
Depends on: 1377174
Attachment #8895273 - Flags: review?(mconley)
Hi Mike,

Could you help review the patch?

And this is the RWD spec[1].

[1]: https://mozilla.invisionapp.com/share/X8BGCX9PD#/screens/245152762
Comment on attachment 8895273 [details]
Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px.

https://reviewboard.mozilla.org/r/166444/#review171640

::: browser/themes/shared/incontentprefs/preferences.inc.css:17
(Diff revision 1)
>  }
>  
>  .pane-container {
>    display: block;
> -  max-width: 800px;
> +  max-width: 664px;
> +  min-width: 664px;

Hi Mike,

I'm not sure why need to use `max-width: 664px;` and `min-width: 664px;` to keep the width fixed as 664px. Using `width: 664px;` (without `max-width` and `min-width`) doesn't work for setting the width.

Do you know why?
Comment on attachment 8895273 [details]
Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px.

https://reviewboard.mozilla.org/r/166444/#review171640

> Hi Mike,
> 
> I'm not sure why need to use `max-width: 664px;` and `min-width: 664px;` to keep the width fixed as 664px. Using `width: 664px;` (without `max-width` and `min-width`) doesn't work for setting the width.
> 
> Do you know why?

If I had to guess, I'd say "XUL box model". jaws might have a more definitive answer - been a while since I've had to deal with XUL sizing, I'm afraid.
Hi Jared,

Do you think it make sense we use the below css rules in the patch[1] to keep the `.pane-container` element have fixed width as 664?
```
.pane-container {
  display: block;
  max-width: 664px;
  min-width: 664px;
}
```

I tried to use the below css rules to do so but it doesn't work. Do you have any other idea to let the `.pane-container` element have a fixed width in this case?
```
.pane-container {
  display: block;
  width: 664px;
}
```

Thank you.

[1]: https://reviewboard.mozilla.org/r/166444/diff/1#index_header
Flags: needinfo?(jaws)
Comment on attachment 8895273 [details]
Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px.

https://reviewboard.mozilla.org/r/166444/#review172176

::: browser/themes/shared/incontentprefs/preferences.inc.css:11
(Diff revision 1)
>  @namespace html "http://www.w3.org/1999/xhtml";
>  
>  .main-content {
>    padding-top: 0;
>    background-color: #fafafc;
> +  padding-inline-start: 28px !important;

Please make this change in common.inc.css.
Blocks: 1388414
(In reply to Mike Conley (:mconley) from comment #4)
> Comment on attachment 8895273 [details]
> Bug 1386514 - Implement RWD to match Photon visual spec.
> 
> https://reviewboard.mozilla.org/r/166444/#review171640
> 
> > Hi Mike,
> > 
> > I'm not sure why need to use `max-width: 664px;` and `min-width: 664px;` to keep the width fixed as 664px. Using `width: 664px;` (without `max-width` and `min-width`) doesn't work for setting the width.
> > 
> > Do you know why?
> 
> If I had to guess, I'd say "XUL box model". jaws might have a more
> definitive answer - been a while since I've had to deal with XUL sizing, I'm
> afraid.

This node doesn't use XUL layout, because display: block is used here. Mixing XUL and non-XUL layout is generally fragile. Why was display: block used here?
Comment on attachment 8895273 [details]
Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px.

https://reviewboard.mozilla.org/r/166444/#review172582

::: browser/themes/shared/incontentprefs/preferences.inc.css:11
(Diff revision 1)
>  @namespace html "http://www.w3.org/1999/xhtml";
>  
>  .main-content {
>    padding-top: 0;
>    background-color: #fafafc;
> +  padding-inline-start: 28px !important;

Sure, let's do it. Thank you for pointing out.
(In reply to Dão Gottwald [::dao] from comment #8)
> (In reply to Mike Conley (:mconley) from comment #4)
> This node doesn't use XUL layout, because display: block is used here.
> Mixing XUL and non-XUL layout is generally fragile. Why was display: block
> used here?

Hi Dão,

According to my understand, `display: block` is a workaround[1] to keep the container always float on the `top: 0` which added at Bug 1377009. 

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1377009#c11
Comment on attachment 8895273 [details]
Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px.

https://reviewboard.mozilla.org/r/166444/#review171640

> If I had to guess, I'd say "XUL box model". jaws might have a more definitive answer - been a while since I've had to deal with XUL sizing, I'm afraid.

Fixed it by adding `align=start` into it's parent node since the default vaule of `align` is `stretch` which means the child elements are stretched to fit the size of the parent elements.
Hi Mike,

Fixed the `max-width` and `min-width` issue. Could you help review it again?

Thank you.
Flags: needinfo?(jaws)
Attachment #8895273 - Flags: review?(mconley) → review?(jaws)
Hi Jared,

Since Mike is on PTO, could you help review the patch?

Thank you.
Comment on attachment 8895273 [details]
Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px.

https://reviewboard.mozilla.org/r/166444/#review173484

::: browser/themes/shared/incontentprefs/preferences.inc.css:15
(Diff revision 6)
> -  max-width: 800px;
> +  width: 664px;
> +  min-width: 664px;

I have seen the spec but I don't understand why we need a min-width here? Much of the content will already resize correctly to narrower widths. This seems to make the experience for people on small resolutions worse.

For example, on Nightly 46.0a1 (2016-01-01), the full window of the preferences resizes to an innerWidth of 517px before horizontal scrollbars appear.

This patch sets a min-width of 664px just on the .pane-container, which makes the min-width of the full window larger than 720px.

::: browser/themes/shared/incontentprefs/preferences.inc.css:161
(Diff revision 6)
> +  #categories {
> +    width: 118px;
> +  }
> +  .category,
> +  .category:hover,
> +  .category[selected] {
> +    padding-inline-start: 47px; /* make category icons align center */

In small windows, why do we use so much space to pad the category icons? This takes away useful space from .pane-container.
Attachment #8895273 - Flags: review?(jaws) → review-
Comment on attachment 8895273 [details]
Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px.

https://reviewboard.mozilla.org/r/166444/#review173486

::: commit-message-9f812:1
(Diff revision 6)
> +Bug 1386514 - Implement RWD to match Photon visual spec.

Please explain more of what the patch is doing. "RWD" as in "Responsive Web Design" is not detailed enough.
Comment on attachment 8895273 [details]
Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px.

https://reviewboard.mozilla.org/r/166444/#review173786

::: browser/themes/shared/incontentprefs/preferences.inc.css:161
(Diff revision 6)
> +  #categories {
> +    width: 118px;
> +  }
> +  .category,
> +  .category:hover,
> +  .category[selected] {
> +    padding-inline-start: 47px; /* make category icons align center */

Can we move this changes to common.inc.css?
Bug 1377174 is moving category changes to common.inc.css. For instance, `width: 240px` will be added in *|*.category within common.inc.css. Other in-content pages would be broken if RWD style only apply in preferences.inc.css.
Comment on attachment 8895273 [details]
Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px.

https://reviewboard.mozilla.org/r/166444/#review173872

::: browser/themes/shared/incontentprefs/preferences.inc.css:15
(Diff revision 6)
> -  max-width: 800px;
> +  width: 664px;
> +  min-width: 664px;

Hi Jared,

According to my understand of the spec, it require a fixed as 664px width for the right side's content.

But you make a good point here. And this is a design issue. Let's hear feedback from designer, Helen. But she is on PTO today. Let's discuss this when she is back.

::: browser/themes/shared/incontentprefs/preferences.inc.css:161
(Diff revision 6)
> +  #categories {
> +    width: 118px;
> +  }
> +  .category,
> +  .category:hover,
> +  .category[selected] {
> +    padding-inline-start: 47px; /* make category icons align center */

To Jared,

It is also a design issue. Let's wait for Helen's feedback.

To Ricky,
Sure, let's do it.
Hi Helen,

What do you think of Comment 19?
And let's discuss it in person tomorrow when you have time.
Thank you very much.
Flags: needinfo?(hhuang)
Discussed with Helen in person this morning, she is considering the details of the visual design change for min-width (we might use `min-width: 530px` here) and category list issues in Comment 19 and and will reply it later.

And let's see where we want to go and get feedback form Jared, and let's update the patch for it.
Attached image screenshot.png (deleted) — 
I see Jared's point that set 664px as mini-width will work badly on the small resolution. Per discussion with Evan, we use Privacy & Security page as an example. When the minimum width of the content area is 530px, the characters of the shortest sentence is around 47 per line, and it could still provide a good reading experience (ideal range is 40-60), so maybe it's okay to set 530px as min-width. 

Jared, Is it okay for you?
Flags: needinfo?(hhuang) → needinfo?(jaws)
Attached image screenshot.png showing category width (deleted) — 
Yes, increasing to 530px is an improvement.

Do you think we would be able to reduce the amount of space between the category list and the pane content? In your screenshot, there is somewhere around 30px more space to the right of the category icons than to the left of the category icons. Ideally we should balance this, right?

I have attached a screenshot showing my measurements.
Flags: needinfo?(jaws)
Rebased and updated patch for Comment 26. The last thing we need to make a decision and do is fixing the Comment 27. Will discuss it with Helen in person later.
hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
searching for changes
remote: abort: abandoned transaction found!
remote: (run 'hg recover' to clean up transaction)
abort: stream ended unexpectedly (got 0 bytes, expected 4)
Hi Jared,

Discussed with Helen in person, we think we could remove the 28px padding when on the small window (>= 960px) mode. She will update the spec when she has time. Thank you, Helen.

And I've updated the patch for our conclusions. Could you please review it again?
Thank you.
Flags: needinfo?(hhuang)
I've modified the spec accordingly, please see the updated spec https://mozilla.invisionapp.com/share/X8BGCX9PD#/245152761_RWD_-_W840. Thanks!
Flags: needinfo?(hhuang)
Comment on attachment 8895273 [details]
Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px.

https://reviewboard.mozilla.org/r/166444/#review174986

This looks mostly OK, but the lack of padding between .pane-content and the edge of the window when the window is narrow doesn't look polished. See https://www.screencast.com/t/sHlkH0wIs for a screenshot.

If we can add 10px of padding there, and if Helen is OK with it, then I think this will be good to land. The spec doesn't show how the window should look when we have a narrow window and we are not cutting off the content, so it is not obvious what was desired in this state.
Attachment #8895273 - Flags: review?(jaws) → review-
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #34)
> This looks mostly OK, but the lack of padding between .pane-content and the
> edge of the window when the window is narrow doesn't look polished. See
> https://www.screencast.com/t/sHlkH0wIs for a screenshot.
> 
> If we can add 10px of padding there, and if Helen is OK with it, then I
> think this will be good to land. The spec doesn't show how the window should
> look when we have a narrow window and we are not cutting off the content, so
> it is not obvious what was desired in this state.

Sure, let's get feedback from Helen.
Flags: needinfo?(hhuang)
That makes sense to me, I will update the spec.
Thanks for your suggestion, Jared.
Flags: needinfo?(hhuang)
Comment on attachment 8895273 [details]
Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px.

https://reviewboard.mozilla.org/r/166444/#review175172

::: toolkit/themes/shared/in-content/common.inc.css:696
(Diff revision 8)
> +  .category,
> +  .category:hover,
> +  .category[selected] {

Why do we need extra selectors for `.category:hover` and `.category[selected]`? Shouldn't `.category` be good enough?
Comment on attachment 8895273 [details]
Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px.

https://reviewboard.mozilla.org/r/166444/#review175192

::: toolkit/themes/shared/in-content/common.inc.css:696
(Diff revision 8)
> +  .category,
> +  .category:hover,
> +  .category[selected] {

You're right. Let's update it.
Hi Jared,

I've updated patch for review comments and added the 10px margin.

Could you review it again?

Thank you.
Comment on attachment 8895273 [details]
Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px.

https://reviewboard.mozilla.org/r/166444/#review175462

r=me if you move the margin-inline-end to .pane-container

::: toolkit/themes/shared/in-content/common.inc.css:710
(Diff revisions 8 - 11)
>    }
>  
>    .main-content {
>      padding-left: 0;
>      padding-right: 0;
> +    margin-inline-end: 10px;

When I make the window narrower this margin ends up on the right side of the scrollbar which is very odd. 

If I move this margin-inline-end to .pane-container then the problem is fixed and all looks good.
Attachment #8895273 - Flags: review?(jaws) → review+
Comment on attachment 8895273 [details]
Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px.

https://reviewboard.mozilla.org/r/166444/#review175462

> When I make the window narrower this margin ends up on the right side of the scrollbar which is very odd. 
> 
> If I move this margin-inline-end to .pane-container then the problem is fixed and all looks good.

Sure, let's do it.
Updated the patch for the review comment. Let's land it once the try[1] is good.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d76a8f2efbda
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/67ad8661e46d
Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px. r=jaws
https://hg.mozilla.org/mozilla-central/rev/67ad8661e46d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Build ID: 20170905220108
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0

Verified as fixed on Firefox Nightly 57.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: