Layout adjustment for i18n compatibility.

Layout adjustment for i18n compatibility.

Adjust layout of settings pages to use a display table in order to adjust appropriately for internationalization.  Fix vertical alignment of headings with options.

BUG=74487, 56535
TEST=Launch in a variety of locales.  Go to 'chrome://settigns/browser'.  Check for layout problems due to overlap of an option heading and the corresponding controls.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=88419

[kevers, 2011-06-02 19:03:36]

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/advanced_options.html (right):

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/advanced_options.html:3: <div
class="displaytable">
Added new <div> block to use 'display:table'.  Remaining edit is an indent
adjustment.

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/browser_options.html (right):

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/browser_options.html:3: <div
class="displaytable">
Added new <div> block to use 'display:table'.  Remaining edit is an indent
adjustment.

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/chromeos/accounts_options.html (right):

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/chromeos/accounts_options.html:3: <div
class="displaytable"  id="userOptionsTable">
Added <div> to make use of 'display:table' for i18n friendly layout.  Remainder
of edit to this file is indent adjustment.

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/chromeos/accounts_options_page.css
(right):

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/chromeos/accounts_options_page.css:76:
#userOptionsTable {
The label 'These settings may only be modified by the owner:' was too tight to
the header.

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/chromeos/internet_options.html (right):

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/chromeos/internet_options.html:7: <div
class="displaytable">
Added <div> to make use of 'display:table' for i18n friendly layout.  Remainder
of edit to this file is indent adjustment.

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/chromeos/system_options.html (right):

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/chromeos/system_options.html:3: <div
class="displaytable">
Added <div> to make use of 'display:table' for i18n friendly layout.  Remainder
of edit to this file is indent adjustment.

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/personal_options.css (right):

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/personal_options.css:8: vertical-align: middle;
This fix allows the use of the checkbox text to the right of the icon as a
baseline for alignment of the section heading.  The checkbox text is pulled in a
bit tighter to the icon than with the table layout.

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/personal_options.html (right):

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/personal_options.html:3: <div
class="displaytable">
Added <div> to make use of 'display:table' for i18n friendly layout. Remainder
of the change is primarily an indentation change with the exception of the
"account" section, which was modified to fix vertical alignment.

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/personal_options.html:7: <div>
Changed from using a table to fix vertical alignment of the section heading.

[Rick Byers, 2011-06-03 15:06:06]

I'm glad Tab's suggestion of using tables was relatively easy way to finally
avoid the fixed width-war we've been struggling with here, good work!

I'd like to see if we can simplify / clean up the CSS rules a little bit though
to make this a little easier to understand and maintain.

Rick

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/advanced_options.html (right):

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/advanced_options.html:3: <div
class="displaytable">
On 2011/06/02 19:03:36, kevers wrote:
> Added new <div> block to use 'display:table'.  Remaining edit is an indent
> adjustment.

Wow, it's lame that rietveld has no way to view a diff ignoring whitespace (or
at least weight whitespace lower when determining the best alignment).  I
applied the diff to my own repro ('git cl patch 7088010') and did a diff
ignoring whitepsace ('git diff -b trunk') just as a double-check.

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/advanced_options.html:13: <div
i18n-content="improveBrowsingExperience" class="informational-text">
I believe we generally try to keep html to 80 cols.  Please split these lines
like some of the other tages in the file already (eg. see line 18 - split at an
attribute and indent 4 spaces).

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/chromeos/accounts_options.html (right):

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/chromeos/accounts_options.html:3: <div
class="displaytable"  id="userOptionsTable">
Remote extra space before id

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/chromeos/accounts_options_page.css
(right):

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/chromeos/accounts_options_page.css:76:
#userOptionsTable {
On 2011/06/02 19:03:36, kevers wrote:
> The label 'These settings may only be modified by the owner:' was too tight to
> the header.

Is it possible to fix this instead by modifying the #ownerOnlyWarning below
(which currently doesn't have any top margin) instead of applying a margin to
the entire userOptionsTable?  Ideally the fix should be made to the element that
is directly problematic, for example incase someone rearranges the elements. 
It's also nice to avoid introducing a new ID if possible.

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/options_page.css (right):

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.css:355: 
Why the extra newline here and below?

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.css:785: * in multiple locales. 
The left column should expand to accomodate the
Remove trailing whitespace at the end of this line and next (unfortunately
rietveld doesn't highlight this - use git diff to see it).

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.css:787: * languages such as
German or Dutch tend to be wider than for English */
move */ to a new line like the comments above

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.css:790: width:100%;
space after :

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.css:793: .displaytable > section {
Are all sections now inside a displaytable?  Or are there still some cases where
you haven't put sections inside of tables.  If your goal is to reformat all
options to use your table layout, then I'd find it cleaner if you integrated
these rules into the rules above rather than just append a bunch of rules
qualified on .displaytable.  For one thing, descendent selectors apparently add
a significant perf overhead (I believe our style guide says to minimize the use
of them as a result).  But more generally, the fewer new CSS rules we have to
add the simpler and more understandable the code is.

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.css:798: .displaytable > section >
h3 {
Again this seems somewhat redundant with the section > h3 rule above.  If you
really do need to handle ".displaytable > section > h3" differently from other
"section >h3" elements, then you should at least try to avoid redundancy.  Eg.,
it looks like font-weight:bold is redundant - it'll pick it up from the above
(more general) rule, so there's hopefully no need to respecify it here.

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.css:804: -webkit-box-flex: 1;
Does this actually do anything for display: table-cell?  I thought
-webkit-box-flex only applied to display: -webkit-box

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.css:806: border-bottom: 1px solid
#eeeeee;
I'm hoping also that it's not necessary to respecify the border.  If we want to
change the color of this rule some day, there were previously only one or two
places we had to change it, now there are 2 more.  Can you avoid repeating this?

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.css:810: .displaytable > section >
h3.centerAlign {
What does this affect? Perhaps I missed it, but I don't see any elements
anywhere with a class of "centerAlign"

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.css:822: border-bottom: 1px solid
#eeeeee;
Again - would be better if you can avoid repeating the border.  This wasn't
necessary before, why is it necessary now?  Doing this also forces you to add
the rules below, which would ideally already be covered by the single general
border-bottom: none rule on line 367.

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.css:830: 
Remove 4 trailing new-lines

[kevers1, 2011-06-03 18:45:44]

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/chromeos/accounts_options.html (right):

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/chromeos/accounts_options.html:3: <div
class="displaytable"  id="userOptionsTable">
On 2011/06/03 15:06:07, rbyers wrote:
> Remote extra space before id

id no longer necessary.

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/chromeos/accounts_options_page.css
(right):

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/chromeos/accounts_options_page.css:76:
#userOptionsTable {
On 2011/06/03 15:06:07, rbyers wrote:
> On 2011/06/02 19:03:36, kevers wrote:
> > The label 'These settings may only be modified by the owner:' was too tight
to
> > the header.
> 
> Is it possible to fix this instead by modifying the #ownerOnlyWarning below
> (which currently doesn't have any top margin) instead of applying a margin to
> the entire userOptionsTable?  Ideally the fix should be made to the element
that
> is directly problematic, for example incase someone rearranges the elements. 
> It's also nice to avoid introducing a new ID if possible.

margin moved to #ownerOnlyWarning

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/options_page.css (right):

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.css:793: .displaytable > section {
On 2011/06/03 15:06:07, rbyers wrote:
> Are all sections now inside a displaytable?  Or are there still some cases
where
> you haven't put sections inside of tables.  If your goal is to reformat all
> options to use your table layout, then I'd find it cleaner if you integrated
> these rules into the rules above rather than just append a bunch of rules
> qualified on .displaytable.  For one thing, descendent selectors apparently
add
> a significant perf overhead (I believe our style guide says to minimize the
use
> of them as a result).  But more generally, the fewer new CSS rules we have to
> add the simpler and more understandable the code is.

The pages within 'toplevel-page-container' have been updated to put the sections
inside of 'displaytable'; however, there is still a fair number of pages that
contain sections without the 'displaytable' wrapper.  For now, we need a
specialized rule.

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.css:806: border-bottom: 1px solid
#eeeeee;
On 2011/06/03 15:06:07, rbyers wrote:
> I'm hoping also that it's not necessary to respecify the border.  If we want
to
> change the color of this rule some day, there were previously only one or two
> places we had to change it, now there are 2 more.  Can you avoid repeating
this?

Ideally, the border would be specified on the section; however, border does not
work with 'display: table-row'.

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.css:810: .displaytable > section >
h3.centerAlign {
On 2011/06/03 15:06:07, rbyers wrote:
> What does this affect? Perhaps I missed it, but I don't see any elements
> anywhere with a class of "centerAlign"

Rule reumoved

[Rick Byers, 2011-06-04 18:49:44]

Thanks for making those improvements Kevin.  I've got a few more suggestions.

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/options_page.css (right):

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.css:793: .displaytable > section {
On 2011/06/03 18:45:44, kevers1 wrote:
> On 2011/06/03 15:06:07, rbyers wrote:
> > Are all sections now inside a displaytable?  Or are there still some cases
> where
> > you haven't put sections inside of tables.  If your goal is to reformat all
> > options to use your table layout, then I'd find it cleaner if you integrated
> > these rules into the rules above rather than just append a bunch of rules
> > qualified on .displaytable.  For one thing, descendent selectors apparently
> add
> > a significant perf overhead (I believe our style guide says to minimize the
> use
> > of them as a result).  But more generally, the fewer new CSS rules we have
to
> > add the simpler and more understandable the code is.
> 
> The pages within 'toplevel-page-container' have been updated to put the
sections
> inside of 'displaytable'; however, there is still a fair number of pages that
> contain sections without the 'displaytable' wrapper.  For now, we need a
> specialized rule.

I see - thanks.

http://codereview.chromium.org/7088010/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.css:806: border-bottom: 1px solid
#eeeeee;
On 2011/06/03 18:45:44, kevers1 wrote:
> On 2011/06/03 15:06:07, rbyers wrote:
> > I'm hoping also that it's not necessary to respecify the border.  If we want
> to
> > change the color of this rule some day, there were previously only one or
two
> > places we had to change it, now there are 2 more.  Can you avoid repeating
> this?
> 
> Ideally, the border would be specified on the section; however, border does
not
> work with 'display: table-row'.

Ah, I see - that's too bad.  Then can you at least combine the rules to avoid
the redundancy?  Eg. you should just be able to have two rules with multiple
selectors describing the border for both the table and non-table cases - maybe
something like:

section,
/* Note: borders are ignored on display:table-row, so we need to re-apply to the
table-cell */
.displaytable > section > * {
  border-bottom: 1px solid #eeeeee;
}

div.page section:last-child,
.displaytable > setion:last-child > * {
  border-bottom: none;
}

It's also fine to explicitly list the children of section as you are now if you
want - I'm not sure whether using the universal selector ('*') is better or not
- just seems simpler/shorter to me.

It you want to avoid combining the table and non-table rules for now (since
they'll eventually be merged) then that's OK too - just merge all your rules so
you're only specifying the border one additional time instead of two.

http://codereview.chromium.org/7088010/diff/7002/chrome/browser/resources/opt...
File chrome/browser/resources/options/advanced_options.html (right):

http://codereview.chromium.org/7088010/diff/7002/chrome/browser/resources/opt...
chrome/browser/resources/options/advanced_options.html:13: <div
i18n-content="improveBrowsingExperience"
Remove trailing whitespace here and 3 other lines (I typically configure my
editor to strip trailing whitespace for me - git diff will also highlight it in
red).

http://codereview.chromium.org/7088010/diff/7002/chrome/browser/resources/opt...
File chrome/browser/resources/options/options_page.css (right):

http://codereview.chromium.org/7088010/diff/7002/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.css:356: 
Remove extra blank line

http://codereview.chromium.org/7088010/diff/7002/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.css:369: 
Remove extra blank line

http://codereview.chromium.org/7088010/diff/7002/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.css:792: * languages such as
German or Dutch tend to be wider than for English
Remove trailing whitespace

http://codereview.chromium.org/7088010/diff/7002/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.css:808: font-weight: bold;
font-size and font-weight appear redundant with the 'section > h3' rule.  Can
you remove them so that this rule specifies only the additional changes
necessary for the table case?

http://codereview.chromium.org/7088010/diff/7002/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.css:817: display: table-cell;
You can combine all the table-cell rules (both left and right columns) together
as well to avoid repetition, eg:
.displaytable > section > * {
  display: table-cell;
  vertical-align: baseline;
}

[James Hawkins, 2011-06-08 17:40:41]

LGTM

FYI, breaking this up into a smaller CL would have been ideal.

[Rick Byers, 2011-06-08 17:55:36]

Thanks for the tweaks Kevin.  One little one left.

Rick

http://codereview.chromium.org/7088010/diff/17004/chrome/browser/resources/op...
File chrome/browser/resources/options/options_page.css (right):

http://codereview.chromium.org/7088010/diff/17004/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.css:817: border-bottom: 1px solid
#eeeeee;
Can you also put display: table-cell and vertical-align: baseline here?  Then
you could remove the '.display > section > h3' rule and also omit those two
lines from the above rule.  I think that will take care of all the redundant
entries here.

[kevers, 2011-06-08 20:39:08]

http://codereview.chromium.org/7088010/diff/17004/chrome/browser/resources/op...
File chrome/browser/resources/options/options_page.css (right):

http://codereview.chromium.org/7088010/diff/17004/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.css:817: border-bottom: 1px solid
#eeeeee;
On 2011/06/08 17:55:36, rbyers wrote:
> Can you also put display: table-cell and vertical-align: baseline here?  Then
> you could remove the '.display > section > h3' rule and also omit those two
> lines from the above rule.  I think that will take care of all the redundant
> entries here.

Done.

[Rick Byers, 2011-06-08 20:59:25]

On 2011/06/08 20:39:08, kevers wrote:
>
http://codereview.chromium.org/7088010/diff/17004/chrome/browser/resources/op...
> File chrome/browser/resources/options/options_page.css (right):
> 
>
http://codereview.chromium.org/7088010/diff/17004/chrome/browser/resources/op...
> chrome/browser/resources/options/options_page.css:817: border-bottom: 1px
solid
> #eeeeee;
> On 2011/06/08 17:55:36, rbyers wrote:
> > Can you also put display: table-cell and vertical-align: baseline here? 
Then
> > you could remove the '.display > section > h3' rule and also omit those two
> > lines from the above rule.  I think that will take care of all the redundant
> > entries here.
> 
> Done.

Thanks!  

LGTM

[commit-bot: I haz the power, 2011-06-08 22:32:15]

Change committed as 88419