WebUI "Edit Search Engine" Dialog now wider and deeper and buttons bottom justified

Dialog boxes now wider and deeper and buttons bottom justified.

BUG=103949
TEST=manual


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

[Kevin Greer, 2011-11-29 19:22:35]

Please review.  Changed width and height and bottom-justified buttons.

[arv (Not doing code reviews), 2011-11-29 20:25:57]

http://codereview.chromium.org/8729042/diff/1/chrome/browser/resources/edit_s...
File chrome/browser/resources/edit_search_engine_dialog.css (right):

http://codereview.chromium.org/8729042/diff/1/chrome/browser/resources/edit_s...
chrome/browser/resources/edit_search_engine_dialog.css:40: bottom: 0px;
s/0px/0/

http://codereview.chromium.org/8729042/diff/1/chrome/browser/resources/edit_s...
chrome/browser/resources/edit_search_engine_dialog.css:43: position: absolute;
This seems a bit strange to me. Why do we want to position: absolute instead of
just increasing the margin-top.

http://codereview.chromium.org/8729042/diff/1/chrome/browser/resources/edit_s...
chrome/browser/resources/edit_search_engine_dialog.css:44: right: 0px;
RTL?

html[dir=rtl] .action-area {
  left: 0;
  right: auto;
}

http://codereview.chromium.org/8729042/diff/1/chrome/browser/resources/edit_s...
chrome/browser/resources/edit_search_engine_dialog.css:45: text-align: end;
text-align is not needed if we go with position absolute

[Kevin Greer, 2011-11-29 21:44:53]

Updated CSS to remove stale attributes and add RTL support to action area.

http://codereview.chromium.org/8729042/diff/1/chrome/browser/resources/edit_s...
File chrome/browser/resources/edit_search_engine_dialog.css (right):

http://codereview.chromium.org/8729042/diff/1/chrome/browser/resources/edit_s...
chrome/browser/resources/edit_search_engine_dialog.css:40: bottom: 0px;
On 2011/11/29 20:25:57, arv wrote:
> s/0px/0/

Done.

http://codereview.chromium.org/8729042/diff/1/chrome/browser/resources/edit_s...
chrome/browser/resources/edit_search_engine_dialog.css:43: position: absolute;
On 2011/11/29 20:25:57, arv wrote:
> This seems a bit strange to me. Why do we want to position: absolute instead
of
> just increasing the margin-top.

I want the buttons to be a fixed distance from the bottom of the screen,
irregardless of the size of the text above, or if the dialog is re-sized.

http://codereview.chromium.org/8729042/diff/1/chrome/browser/resources/edit_s...
chrome/browser/resources/edit_search_engine_dialog.css:44: right: 0px;
On 2011/11/29 20:25:57, arv wrote:
> RTL?
> 
> html[dir=rtl] .action-area {
>   left: 0;
>   right: auto;
> }

Done.

http://codereview.chromium.org/8729042/diff/1/chrome/browser/resources/edit_s...
chrome/browser/resources/edit_search_engine_dialog.css:45: text-align: end;
On 2011/11/29 20:25:57, arv wrote:
> text-align is not needed if we go with position absolute
removed

[arv (Not doing code reviews), 2011-11-29 21:55:30]

http://codereview.chromium.org/8729042/diff/5001/chrome/browser/resources/edi...
File chrome/browser/resources/edit_search_engine_dialog.css (right):

http://codereview.chromium.org/8729042/diff/5001/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.css:44: bottom: 0;
no need to duplicate bottom, padding nor position.

[Rick Byers, 2011-11-29 22:06:47]

http://codereview.chromium.org/8729042/diff/5001/chrome/browser/resources/edi...
File chrome/browser/resources/edit_search_engine_dialog.css (right):

http://codereview.chromium.org/8729042/diff/5001/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.css:39: position: absolute;
I believe the use of webkit-box here was to get the two buttons to be the same
size - both big enough to fit the longer of the two text strings inside them
(regardless of language).  See the comments on the original CL here:
http://codereview.chromium.org/8118012/

It seems like we make an effort to do this elsewhere too (eg. all the buttons on
the options page - see
http://codesearch.google.com/#OAMlx_jo-ck/src/chrome/browser/resources/option...)
so I assume we want to preserve it here too.

It's also nice that using -webkit-box-pack: end avoids the need for a separate
rtl rule.

Can you not just keep all the existing stuff but add "position: fixed, bottom:
0"?

[Rick Byers, 2011-11-29 22:08:27]

Also, please update the title/description:
 - fix typo 'boxex'
 - make it clear this is for the WebUI edit search engine dialog (the fact that
you're tweaking that dialog is the most important thing to get across in the
first line of the description - so people can scan commits)

[Kevin Greer, 2011-11-30 15:42:29]

On 2011/11/29 22:06:47, Rick Byers wrote:
>
http://codereview.chromium.org/8729042/diff/5001/chrome/browser/resources/edi...
> File chrome/browser/resources/edit_search_engine_dialog.css (right):
> 
>
http://codereview.chromium.org/8729042/diff/5001/chrome/browser/resources/edi...
> chrome/browser/resources/edit_search_engine_dialog.css:39: position: absolute;
> I believe the use of webkit-box here was to get the two buttons to be the same
> size - both big enough to fit the longer of the two text strings inside them

With the original CSS, the two buttons are not the same size.  The Cancel button
is wider.  I tested with very long button text and it still displays properly.

> (regardless of language).  See the comments on the original CL here:
> http://codereview.chromium.org/8118012/

My understanding of the CL comments was that they were referring to vertically
stacked buttons being forced to the same width, but in this case, the buttons
are horizontal.

> It seems like we make an effort to do this elsewhere too (eg. all the buttons
on
> the options page - see
>
http://codesearch.google.com/#OAMlx_jo-ck/src/chrome/browser/resources/option...)
> so I assume we want to preserve it here too.
> 
> It's also nice that using -webkit-box-pack: end avoids the need for a separate
> rtl rule.

I agree, but using the webkit-box I couldn't find anyway to also get the buttons
to bottom-justify.  I see that a number of the other CSS have used 'rtl' when
required. 

> Can you not just keep all the existing stuff but add "position: fixed, bottom:
> 0"?

Once I add "position: fixed" it ignores all of the *box* attributes.

[Rick Byers, 2011-11-30 17:29:13]

On 2011/11/30 15:42:29, Kevin Greer wrote:
> On 2011/11/29 22:06:47, Rick Byers wrote:
> >
>
http://codereview.chromium.org/8729042/diff/5001/chrome/browser/resources/edi...
> > File chrome/browser/resources/edit_search_engine_dialog.css (right):
> > 
> >
>
http://codereview.chromium.org/8729042/diff/5001/chrome/browser/resources/edi...
> > chrome/browser/resources/edit_search_engine_dialog.css:39: position:
absolute;
> > I believe the use of webkit-box here was to get the two buttons to be the
same
> > size - both big enough to fit the longer of the two text strings inside them
> 
> With the original CSS, the two buttons are not the same size.  The Cancel
button
> is wider.  I tested with very long button text and it still displays properly.

Ah, you're right - it doesn't work here.  The dialog on the options page I was
looking at (that uses the same webkit-box layout) achieves this (more or less)
with min-width: 87px.  Maybe it doesn't matter.  If it does, we should use
min-width like the options page.

> > (regardless of language).  See the comments on the original CL here:
> > http://codereview.chromium.org/8118012/
> 
> My understanding of the CL comments was that they were referring to vertically
> stacked buttons being forced to the same width, but in this case, the buttons
> are horizontal.

No, that's what Erik initially thought, but Chad corrected him - look at the
next comment down and the diagram Chad drew in ASCII art.

> > It seems like we make an effort to do this elsewhere too (eg. all the
buttons
> on
> > the options page - see
> >
>
http://codesearch.google.com/#OAMlx_jo-ck/src/chrome/browser/resources/option...)
> > so I assume we want to preserve it here too.
> > 
> > It's also nice that using -webkit-box-pack: end avoids the need for a
separate
> > rtl rule.
> 
> I agree, but using the webkit-box I couldn't find anyway to also get the
buttons
> to bottom-justify.  I see that a number of the other CSS have used 'rtl' when
> required. 

I just tried the following rule additions and it worked for me:

.action-area {
  position: fixed;
  bottom: 0;
  width: 100%;
  -webkit-box-sizing: border-box;
}

I also verified that when I switch to RTL (just set html[dir='rtl']) the buttons
move to the left.  I think doing this is better than explicitly relying on
right: 0 / left: 0.

> > Can you not just keep all the existing stuff but add "position: fixed,
bottom:
> > 0"?
> 
> Once I add "position: fixed" it ignores all of the *box* attributes.

I think you want to fix the position of the action-area, not the button-strip. 
It's action-area that has -webkit-box-pack-end that we're relying on to get
things shifted to the right (or left in rtl).

[Kevin Greer, 2011-11-30 21:03:37]

> .action-area {
>   position: fixed;
>   bottom: 0;
>   width: 100%;
>   -webkit-box-sizing: border-box;
> }

I've incorporated this CSS into the code (and removed stale attributes).

[Kevin Greer, 2011-11-30 21:11:17]

Updated with Rick's suggestion for removing explicit 'rtl' rule.  Tested both
RTL and LTR.

[Kevin Greer, 2011-11-30 21:45:33]

The button's min-width is already to set for '4em' so that the button isn't too
small if the button text is less than 4 characters long.  Do you think it should
be further increased to 87px?

[Rick Byers, 2011-12-01 01:34:00]

LGTM
Thanks for switching - this seems nicer to me.

On 2011/11/30 21:45:33, Kevin Greer wrote:
> The button's min-width is already to set for '4em' so that the button isn't
too
> small if the button text is less than 4 characters long.  Do you think it
should
> be further increased to 87px?

I wouldn't bother - the buttons look fine to me.

[commit-bot: I haz the power, 2011-12-01 16:44:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kgr@chromium.org/8729042/14001

[commit-bot: I haz the power, 2011-12-01 16:44:26]

Presubmit check for 8729042-14001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/resources/edit_search_engine_dialog.css

Presubmit checks took 1.4s to calculate.

[Kevin Greer, 2011-12-01 16:46:33]

Hi Rob, can I get at LGTM on this?

[flackr, 2011-12-01 16:49:19]

On 2011/12/01 16:46:33, Kevin Greer wrote:
> Hi Rob, can I get at LGTM on this?

I'm not an owner of chrome/browser/resources, you'll need one of arv, jhawkins
or estade.

[Kevin Greer, 2011-12-01 16:51:45]

Hi Arv,  Can I get an LGTM on this?  Thx

[arv (Not doing code reviews), 2011-12-01 17:58:36]

lgtm

[commit-bot: I haz the power, 2011-12-01 17:59:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kgr@chromium.org/8729042/14001

[commit-bot: I haz the power, 2011-12-01 19:58:58]

Change committed as 112501