Issue 9609023: css hack n' slash, continued: New styles for checkboxes and radios.

Issue 9609023: css hack n' slash, continued: New styles for checkboxes and radios. (Closed)

Created:
8 years, 9 months ago by Evan Stade

Modified:
8 years, 9 months ago

Reviewers:
Patrick Dubroy, csilv, Dan Beam

CC:
chromium-reviews, asanka, dbeam+watch-ntp_chromium.org, nkostylev+watch_chromium.org, yoshiki+watch_chromium.org, Randy Smith (Not in Mondays), Patrick Dubroy, arv (Not doing code reviews), estade+watch_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

css hack n' slash, continued: New styles for checkboxes and radios. BUG=103688, 115404 TEST=visual Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=125688

Patch Set 1 #

Total comments: 24

Patch Set 2 : scale with font #

Patch Set 3 : back to non-scaling inputs to fix chromeos #

Total comments: 11

Patch Set 4 : remove double include #

Patch Set 5 : dbeam review again #

Patch Set 6 : fix couple rtl bugs #

Total comments: 2

Patch Set 7 : csilv review #

Created: 8 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+187 lines, -449 lines)			Patch
M	chrome/browser/resources/bookmark_manager/bookmark_all_tabs.html	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/resources/chromeos/choose_mobile_network.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/chromeos/login/login.html	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/chromeos/login/oobe.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/chromeos/proxy_settings.html	View		1 chunk	+1 line, -2 lines	0 comments	Download
M	chrome/browser/resources/chromeos/sim_unlock.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/downloads.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/feedback.html	View		1 chunk	+1 line, -2 lines	0 comments	Download
M	chrome/browser/resources/gpu_internals/gpu_internals.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/help/help.html	View	1 2 3	1 chunk	+0 lines, -1 line	0 comments	Download
M	chrome/browser/resources/history/history.css	View	1	1 chunk	+1 line, -5 lines	0 comments	Download
M	chrome/browser/resources/ntp4/new_tab.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/options/options.html	View		1 chunk	+1 line, -2 lines	0 comments	Download
M	chrome/browser/resources/policy.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/print_preview/print_preview.css	View		1 chunk	+1 line, -4 lines	0 comments	Download
M	chrome/browser/resources/print_preview/print_preview.html	View		1 chunk	+1 line, -2 lines	0 comments	Download
M	chrome/browser/resources/print_preview/print_preview_dummy.html	View		1 chunk	+1 line, -2 lines	0 comments	Download
D	chrome/browser/resources/shared/css/button.css	View		1 chunk	+0 lines, -178 lines	0 comments	Download
M	chrome/browser/resources/shared/css/checkbox.css	View		1 chunk	+0 lines, -138 lines	0 comments	Download
M	chrome/browser/resources/shared/css/chrome_shared2.css	View	1 2 3 4 5 6	2 chunks	+1 line, -37 lines	0 comments	Download
A +	chrome/browser/resources/shared/css/widgets.css	View	1 2 3 4 5 6	5 chunks	+159 lines, -54 lines	0 comments	Download
M	chrome/browser/resources/shared/images/check.png	View	1	0 chunks	+-1 lines, --1 lines	0 comments	Download
D	chrome/browser/resources/shared/images/checkmark.png	View	1	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	chrome/browser/resources/shared/js/cr/ui/menu_button.js	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/shared/js/cr/ui/menu_item.js	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/shared_resources.grd	View	1 2 3 4 5 6	2 chunks	+4 lines, -6 lines	0 comments	Download
M	chrome/browser/resources/sync_promo/sync_promo.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/task_manager/includes.js	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/tracing.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/uber/uber_shared.css	View	1 2 3 4 5 6	1 chunk	+3 lines, -3 lines	0 comments	Download

Messages

Total messages: 19 (0 generated)

Expand Messages | Collapse Messages

Dan Beam

I'm not sure widgets.css is the best name as people are likely to confuse this ...

8 years, 9 months ago (2012-03-06 04:47:06 UTC) #2

Evan Stade

I have heard them referred to as form controls, but they aren't actually associated with ...

8 years, 9 months ago (2012-03-06 05:18:15 UTC) #3

Dan Beam

If nobody should be including widgets.css anyways (as they're likely to just include chrome_shared2.css instead), ...

8 years, 9 months ago (2012-03-06 05:50:10 UTC) #4

If nobody should be including widgets.css anyways (as they're likely to just
include chrome_shared2.css instead), it doesn't really matter, but I guess just
from a web development background form_elements.css would be more explanatory
and widgets.css would be more muddied (whereas from a native UI background I
don't think you'd hit the same confusion on form_elements.css, but at the same
time we may have other style sheets that affect form controls).

http://codereview.chromium.org/9609023/diff/1/chrome/browser/resources/shared...
File chrome/browser/resources/shared/css/chrome_shared2.css (right):

http://codereview.chromium.org/9609023/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/css/chrome_shared2.css:9: @import
url("chrome://resources/css/widgets.css");
On 2012/03/06 05:18:15, Evan Stade wrote:
> On 2012/03/06 04:47:07, Dan Beam wrote:
> > wait, _why_ are we importing this?
> 
> so that people who import chrome_shared2.css will get that style as well (?)

I thought there would be more than this, but I assumed people wouldn't know that
shared also included button.css/checkbox.css and would double include like this
-
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/resources/he...

http://codereview.chromium.org/9609023/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/css/chrome_shared2.css:81: /* Header
*********************************************************************/
On 2012/03/06 05:18:15, Evan Stade wrote:
> On 2012/03/06 04:47:07, Dan Beam wrote:
> > Headers?
> 
> what is the verb in this question

s/Header/Headers/ IMO or Header styles as Header makes little sense to me as a
class of objects (it sounds singular, i.e. it should be a #header).

http://codereview.chromium.org/9609023/diff/1/chrome/browser/resources/shared...
File chrome/browser/resources/shared/css/widgets.css (right):

http://codereview.chromium.org/9609023/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/css/widgets.css:18: input[type='radio'] {
On 2012/03/06 05:18:15, Evan Stade wrote:
> On 2012/03/06 04:47:07, Dan Beam wrote:
> > why not group inputs?
> 
> why group inputs? input[type='button'] has a lot more in common with select
than
> input[type='radio']

ok, I see your point

http://codereview.chromium.org/9609023/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/css/widgets.css:217: * these classes in the
form:
On 2012/03/06 05:18:15, Evan Stade wrote:
> On 2012/03/06 04:47:07, Dan Beam wrote:
> > s/form/markup structure/?
> 
> why?

seems less ambiguous - in the form could mean anything, I don't care that much
about it

Patrick Dubroy

Drive-by comment -- http://codereview.chromium.org/9609023/diff/1/chrome/browser/resources/shared/css/chrome_shared2.css File chrome/browser/resources/shared/css/chrome_shared2.css (right): http://codereview.chromium.org/9609023/diff/1/chrome/browser/resources/shared/css/chrome_shared2.css#newcode9 chrome/browser/resources/shared/css/chrome_shared2.css:9: @import url("chrome://resources/css/widgets.css"); On 2012/03/06 05:50:10, Dan ...

8 years, 9 months ago (2012-03-06 18:45:19 UTC) #5

Evan Stade

checkboxes and radio now scale with font. http://codereview.chromium.org/9609023/diff/1/chrome/browser/resources/shared/css/chrome_shared2.css File chrome/browser/resources/shared/css/chrome_shared2.css (right): http://codereview.chromium.org/9609023/diff/1/chrome/browser/resources/shared/css/chrome_shared2.css#newcode9 chrome/browser/resources/shared/css/chrome_shared2.css:9: @import url("chrome://resources/css/widgets.css"); ...

8 years, 9 months ago (2012-03-06 22:54:51 UTC) #6

checkboxes and radio now scale with font.

http://codereview.chromium.org/9609023/diff/1/chrome/browser/resources/shared...
File chrome/browser/resources/shared/css/chrome_shared2.css (right):

http://codereview.chromium.org/9609023/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/css/chrome_shared2.css:9: @import
url("chrome://resources/css/widgets.css");
On 2012/03/06 18:45:19, dubroy wrote:
>   On 2012/03/06 05:50:10, Dan Beam wrote:
> > On 2012/03/06 05:18:15, Evan Stade wrote:
> > > On 2012/03/06 04:47:07, Dan Beam wrote:
> > > > wait, _why_ are we importing this?
> > > 
> > > so that people who import chrome_shared2.css will get that style as well
(?)
> > 
> > I thought there would be more than this, but I assumed people wouldn't know
> that
> > shared also included button.css/checkbox.css and would double include like
> this
> > -
> >
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/resources/he...
> 
> For someone new to the webUI code, I think it would be good if there was a
> single include. Most people will probably just follow the example of an
existing
> file, so as long as we clean up the other files to not do double include, I
> don't think people will be confused.

yea, we should fix double includes and don't do them in the future.

http://codereview.chromium.org/9609023/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/css/chrome_shared2.css:81: /* Header
*********************************************************************/
On 2012/03/06 05:50:10, Dan Beam wrote:
> On 2012/03/06 05:18:15, Evan Stade wrote:
> > On 2012/03/06 04:47:07, Dan Beam wrote:
> > > Headers?
> > 
> > what is the verb in this question
> 
> s/Header/Headers/ IMO or Header styles as Header makes little sense to me as a
> class of objects (it sounds singular, i.e. it should be a #header).

well, I removed this whole section in a previous patch, so I guess it's moot.

http://codereview.chromium.org/9609023/diff/1/chrome/browser/resources/shared...
File chrome/browser/resources/shared/css/widgets.css (right):

http://codereview.chromium.org/9609023/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/css/widgets.css:217: * these classes in the
form:
On 2012/03/06 05:50:10, Dan Beam wrote:
> On 2012/03/06 05:18:15, Evan Stade wrote:
> > On 2012/03/06 04:47:07, Dan Beam wrote:
> > > s/form/markup structure/?
> > 
> > why?
> 
> seems less ambiguous - in the form could mean anything, I don't care that much
> about it

Done.

http://codereview.chromium.org/9609023/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/css/widgets.css:235: /* Don't expand
horizontally: http://crbug.com/112091 */
On 2012/03/06 04:47:07, Dan Beam wrote:
> . at end

Done.

Patrick Dubroy

What's the reason for checkboxes and radio buttons to scale with the font size? The ...

8 years, 9 months ago (2012-03-06 22:58:28 UTC) #7

Evan Stade

On 2012/03/06 22:58:28, dubroy wrote: > What's the reason for checkboxes and radio buttons to ...

8 years, 9 months ago (2012-03-06 23:21:56 UTC) #8

Dan Beam

> What's the reason for checkboxes and radio buttons to scale with the font size? ...

8 years, 9 months ago (2012-03-06 23:28:05 UTC) #9

Dan Beam

thank you for removing double include http://codereview.chromium.org/9609023/diff/10001/chrome/browser/resources/shared/css/widgets.css File chrome/browser/resources/shared/css/widgets.css (left): http://codereview.chromium.org/9609023/diff/10001/chrome/browser/resources/shared/css/widgets.css#oldcode78 chrome/browser/resources/shared/css/widgets.css:78: input[type='button']:not(.custom-appearance):not(.link-button):enabled:hover, why'd you ...

8 years, 9 months ago (2012-03-07 01:01:22 UTC) #10

Evan Stade

fixed rtl as well http://codereview.chromium.org/9609023/diff/10001/chrome/browser/resources/shared/css/widgets.css File chrome/browser/resources/shared/css/widgets.css (left): http://codereview.chromium.org/9609023/diff/10001/chrome/browser/resources/shared/css/widgets.css#oldcode78 chrome/browser/resources/shared/css/widgets.css:78: input[type='button']:not(.custom-appearance):not(.link-button):enabled:hover, On 2012/03/07 01:01:22, Dan ...

8 years, 9 months ago (2012-03-07 02:52:37 UTC) #11

csilv

http://codereview.chromium.org/9609023/diff/10002/chrome/browser/resources/shared/css/widgets.css File chrome/browser/resources/shared/css/widgets.css (right): http://codereview.chromium.org/9609023/diff/10002/chrome/browser/resources/shared/css/widgets.css#newcode279 chrome/browser/resources/shared/css/widgets.css:279: /* NOTE: Keep a relatively high min-width for this ...

8 years, 9 months ago (2012-03-07 19:52:09 UTC) #12

Dan Beam

lgtm http://codereview.chromium.org/9609023/diff/10001/chrome/browser/resources/shared/css/widgets.css File chrome/browser/resources/shared/css/widgets.css (left): http://codereview.chromium.org/9609023/diff/10001/chrome/browser/resources/shared/css/widgets.css#oldcode78 chrome/browser/resources/shared/css/widgets.css:78: input[type='button']:not(.custom-appearance):not(.link-button):enabled:hover, On 2012/03/07 02:52:37, Evan Stade wrote: > ...

8 years, 9 months ago (2012-03-07 20:02:19 UTC) #13

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/9609023/10002

8 years, 9 months ago (2012-03-07 23:10:37 UTC) #14

Evan Stade

8 years, 9 months ago (2012-03-08 00:52:08 UTC) #15

commit-bot: I haz the power

Commit queue failed due to new patchset.

8 years, 9 months ago (2012-03-08 03:38:18 UTC) #17

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/9609023/17001

8 years, 9 months ago (2012-03-08 03:38:37 UTC) #18

commit-bot: I haz the power

8 years, 9 months ago (2012-03-08 05:49:55 UTC) #19

Can't apply patch for file chrome/browser/resources/shared_resources.grd.
While running patch -p1 --forward --force;
patching file chrome/browser/resources/shared_resources.grd
Hunk #1 FAILED at 14.
Hunk #2 succeeded at 44 (offset 2 lines).
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/resources/shared_resources.grd.rej

Expand Messages | Collapse Messages