Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15969014:
History: Update managed user history page (Closed)
|
Created:
7 years, 6 months ago by Sergiu Modified:
7 years, 6 months ago CC:
chromium-reviews, Patrick Dubroy, pam+watch_chromium.org, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionHistory: Update managed user history page
Updates the managed user history page, adding a fixed top menu and possiblity
to show:
- normal "all view".
- grouped by domain "week" view.
- grouped by domain "month" view.
Also shows the managed user filter status for each domain and changes the way
blocked visit attempts are shown.
R=jhawkins@chromium.org
BUG=228844
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=205757
Patch Set 1 #Patch Set 2 : Clean-up #Patch Set 3 : Fixes #Patch Set 4 : Minor fix #
Total comments: 16
Patch Set 5 : Minor fixes #
Total comments: 4
Patch Set 6 : Fixes #
Total comments: 3
Patch Set 7 : RTL fix #
Total comments: 20
Patch Set 8 : Fixes #Patch Set 9 : Rebase #Patch Set 10 : Minor fixes #
Total comments: 2
Patch Set 11 : Indent fix #Patch Set 12 : Image buttons #
Total comments: 1
Patch Set 13 : tag on new line #Patch Set 14 : Rebase #Patch Set 15 : Fix browser tests #
Total comments: 1
Messages
Total messages: 32 (0 generated)
Hey James, please take a look. You can find some screenshots here of what it looks like with this patch: https://drive.google.com/a/google.com/folderview?id=0B6tjSjEmCuvYRkZHQ0c2d3p5...
Shouldn't dubroy take a look as well? https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... File chrome/browser/resources/history/history.html (right): https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... chrome/browser/resources/history/history.html:73: </button> Can this fit on the line above? https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... chrome/browser/resources/history/history.html:77: </button><button id="range-next" disabled> Why is this next button not on a new line? https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... chrome/browser/resources/history/history.html:83: <input type="radio" id="timeframe-filter-all" nit: IDs should be the first attribute. https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... chrome/browser/resources/history/history.js:1041: //$('timeframe-filter').value = this.getRangeInDays(); What is this about?
On 2013/06/04 18:22:05, James Hawkins wrote: > Shouldn't dubroy take a look as well? > > https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... > File chrome/browser/resources/history/history.html (right): > > https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... > chrome/browser/resources/history/history.html:73: </button> > Can this fit on the line above? > > https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... > chrome/browser/resources/history/history.html:77: </button><button > id="range-next" disabled> > Why is this next button not on a new line? > > https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... > chrome/browser/resources/history/history.html:83: <input type="radio" > id="timeframe-filter-all" > nit: IDs should be the first attribute. > > https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... > File chrome/browser/resources/history/history.js (right): > > https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... > chrome/browser/resources/history/history.js:1041: //$('timeframe-filter').value > = this.getRangeInDays(); > What is this about? It'd be great, but he's out of office for the next month and a half and we're trying to hit M29 :( I'll fix the comments tomorrow at work.
Markus offered to take a look as well. https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... File chrome/browser/resources/history/history.html (right): https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... chrome/browser/resources/history/history.html:73: </button> On 2013/06/04 18:22:05, James Hawkins wrote: > Can this fit on the line above? Done. https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... chrome/browser/resources/history/history.html:77: </button><button id="range-next" disabled> On 2013/06/04 18:22:05, James Hawkins wrote: > Why is this next button not on a new line? Well, this is related to the problem of space between inline-block elements. If we have whitespace between two inline elements it gets rendered. The possible solutions to this are: 1. Remove the whitespace as above (simplest). 2. I could set a negative margin of -4px instead of -1px so that it covers the whitespace. 3. Probably could use floats/flex-box for this. (most complicated for sure) I went with 1, but if you think that other options are better I'm open to that as well. https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... chrome/browser/resources/history/history.html:83: <input type="radio" id="timeframe-filter-all" On 2013/06/04 18:22:05, James Hawkins wrote: > nit: IDs should be the first attribute. Done. https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... chrome/browser/resources/history/history.js:1041: //$('timeframe-filter').value = this.getRangeInDays(); On 2013/06/04 18:22:05, James Hawkins wrote: > What is this about? Whoops, that slipped, removed.
On 2013/06/05 11:21:37, Sergiu wrote: > Markus offered to take a look as well. > > https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... > File chrome/browser/resources/history/history.html (right): > > https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... > chrome/browser/resources/history/history.html:73: </button> > On 2013/06/04 18:22:05, James Hawkins wrote: > > Can this fit on the line above? > > Done. > > https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... > chrome/browser/resources/history/history.html:77: </button><button > id="range-next" disabled> > On 2013/06/04 18:22:05, James Hawkins wrote: > > Why is this next button not on a new line? > > Well, this is related to the problem of space between inline-block elements. If > we have whitespace between two inline elements it gets rendered. > The possible solutions to this are: > 1. Remove the whitespace as above (simplest). > 2. I could set a negative margin of -4px instead of -1px so that it covers the > whitespace. > 3. Probably could use floats/flex-box for this. (most complicated for sure) > > I went with 1, but if you think that other options are better I'm open to that > as well. > > https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... > chrome/browser/resources/history/history.html:83: <input type="radio" > id="timeframe-filter-all" > On 2013/06/04 18:22:05, James Hawkins wrote: > > nit: IDs should be the first attribute. > > Done. > > https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... > File chrome/browser/resources/history/history.js (right): > > https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... > chrome/browser/resources/history/history.js:1041: //$('timeframe-filter').value > = this.getRangeInDays(); > On 2013/06/04 18:22:05, James Hawkins wrote: > > What is this about? > > Whoops, that slipped, removed. I used to hate when reviewers requested this but now that I'm reviewing UI code myself I realize how much that helps: Could you please attach a screenshot :)
There's a link to the screenshots in my first post :)
Uups. I didn't see that the mocks were attached to the first set of messages. Thanks!! Some initial comments. https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... File chrome/browser/resources/history/history.html (right): https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... chrome/browser/resources/history/history.html:64: <div class="page" id="page"> Please make id the first attribute. https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... chrome/browser/resources/history/history.js:88: // Shows the filtering behavior for that host (used by a managed user). Is this only for managed mode? If so would you mind improving the comment :) THX https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... chrome/browser/resources/history/history.js:832: var groupedByDomain = (range == HistoryModel.Range.ALL_TIME) ? false : true; This variable is not used in the remainder of the function body. https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... chrome/browser/resources/history/history.js:1163: * Adds the text that shows the current inteval, used for week and month typo: inte_r_val
https://codereview.chromium.org/15969014/diff/17001/chrome/browser/resources/... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/15969014/diff/17001/chrome/browser/resources/... chrome/browser/resources/history/history.js:1475: $('filter-controls').hidden = true; Just for the record since we already discussed this offline: Maybe it is better to to it the other way around and display the filter controls only if in managed mode instead of hiding them for normal mode. I assume that normal mode will still be the more common case.
https://codereview.chromium.org/15969014/diff/17001/chrome/browser/resources/... File chrome/browser/resources/history/history.js (left): https://codereview.chromium.org/15969014/diff/17001/chrome/browser/resources/... chrome/browser/resources/history/history.js:1800: * @param {boolean} inContentPack Whether this element is in a content pack or Just to double check so that I don't misunderstand this: All support for content packs is removed for now. Right? https://codereview.chromium.org/15969014/diff/17001/chrome/browser/resources/... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/15969014/diff/17001/chrome/browser/resources/... chrome/browser/resources/history/history.js:160: titleAndDomainWrapper.classList.remove('title-and-domain'); A) Instead of removing it. Maybe you don't add it in first place? e.g. in line 151: var titleAndDomainWrapper = entryBox.appendChild( addTitleFavicon ? createElementWithClassName('div', 'title-and- domain') : createElement('div')); B) or you can also add the class name in line 158 and line 154. This means that you have to add it twice. The root cause of this is that the class name is bad: 'title-and-domain'. It should be rather 'favicon' or 'had-icon' or something like this, since you seem to only need it if there is an icon used. I guess this makes it easier to follow the code. WDYT?
https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... File chrome/browser/resources/history/history.html (right): https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... chrome/browser/resources/history/history.html:64: <div class="page" id="page"> On 2013/06/05 14:00:06, markusheintz_ wrote: > Please make id the first attribute. Done. https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... chrome/browser/resources/history/history.js:88: // Shows the filtering behavior for that host (used by a managed user). On 2013/06/05 14:00:06, markusheintz_ wrote: > Is this only for managed mode? If so would you mind improving the comment :) THX Done. https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... chrome/browser/resources/history/history.js:832: var groupedByDomain = (range == HistoryModel.Range.ALL_TIME) ? false : true; On 2013/06/05 14:00:06, markusheintz_ wrote: > This variable is not used in the remainder of the function body. Ooops, good catch! https://codereview.chromium.org/15969014/diff/10001/chrome/browser/resources/... chrome/browser/resources/history/history.js:1163: * Adds the text that shows the current inteval, used for week and month On 2013/06/05 14:00:06, markusheintz_ wrote: > typo: inte_r_val Done. https://codereview.chromium.org/15969014/diff/17001/chrome/browser/resources/... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/15969014/diff/17001/chrome/browser/resources/... chrome/browser/resources/history/history.js:160: titleAndDomainWrapper.classList.remove('title-and-domain'); On 2013/06/05 14:49:09, markusheintz_ wrote: > A) > Instead of removing it. Maybe you don't add it in first place? > > e.g. in line 151: > var titleAndDomainWrapper = entryBox.appendChild( > addTitleFavicon ? createElementWithClassName('div', 'title-and- > domain') > : createElement('div')); > > B) > or you can also add the class name in line 158 and line 154. This means that you > have to add it twice. > > > > The root cause of this is that the class name is bad: 'title-and-domain'. It > should be rather 'favicon' or 'had-icon' or something like this, since you seem > to only need it if there is an icon used. > > > > I guess this makes it easier to follow the code. WDYT? I agree, see what you think now of the new names and structure.
LGTM from my side. @James: Could you maybe take a look at the CSS. Just to double check? Thanks https://codereview.chromium.org/15969014/diff/25001/chrome/browser/resources/... File chrome/browser/resources/history/history.html (right): https://codereview.chromium.org/15969014/diff/25001/chrome/browser/resources/... chrome/browser/resources/history/history.html:76: </button><button id="range-next" disabled> I guess James already mentioned this. Could you move the <button> tag in a new line pls.
On 2013/06/06 09:43:51, markusheintz_ wrote: > LGTM from my side. > > @James: Could you maybe take a look at the CSS. Just to double check? > > Thanks > > https://codereview.chromium.org/15969014/diff/25001/chrome/browser/resources/... > File chrome/browser/resources/history/history.html (right): > > https://codereview.chromium.org/15969014/diff/25001/chrome/browser/resources/... > chrome/browser/resources/history/history.html:76: </button><button > id="range-next" disabled> > I guess James already mentioned this. > Could you move the <button> tag in a new line pls. Since these two elements are "display:inline-block" having a whitespace in between of them results in a small gap between the two buttons. Therefore I'm fine with leaving the two buttons on the same line.
On 2013/06/06 15:38:36, markusheintz_ wrote: > On 2013/06/06 09:43:51, markusheintz_ wrote: > > LGTM from my side. > > > > @James: Could you maybe take a look at the CSS. Just to double check? > > > > Thanks > > > > > https://codereview.chromium.org/15969014/diff/25001/chrome/browser/resources/... > > File chrome/browser/resources/history/history.html (right): > > > > > https://codereview.chromium.org/15969014/diff/25001/chrome/browser/resources/... > > chrome/browser/resources/history/history.html:76: </button><button > > id="range-next" disabled> > > I guess James already mentioned this. > > Could you move the <button> tag in a new line pls. > Since these two elements are "display:inline-block" having a whitespace in > between of them results in a small gap between the two buttons. Therefore I'm > fine with leaving the two buttons on the same line. James, it would be great if you could take a look at this today as we're trying to wrap this up by the end of this week. Thanks!
https://codereview.chromium.org/15969014/diff/25001/chrome/browser/resources/... File chrome/browser/resources/history/history.css (right): https://codereview.chromium.org/15969014/diff/25001/chrome/browser/resources/... chrome/browser/resources/history/history.css:109: border-bottom-left-radius: 0; RTL. Essentially any place where you use -left or -right w/out an accompanying html[dir='rtl'], you have an RTL issue. See line 68 for how to do this.
https://codereview.chromium.org/15969014/diff/25001/chrome/browser/resources/... File chrome/browser/resources/history/history.css (right): https://codereview.chromium.org/15969014/diff/25001/chrome/browser/resources/... chrome/browser/resources/history/history.css:109: border-bottom-left-radius: 0; On 2013/06/06 18:58:23, James Hawkins wrote: > RTL. > > Essentially any place where you use -left or -right w/out an accompanying > html[dir='rtl'], you have an RTL issue. See line 68 for how to do this. Done, screenshot here: https://docs.google.com/a/google.com/file/d/0B6tjSjEmCuvYRkQ3VDYwMDhhbk0/edit...
https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... File chrome/browser/resources/history/history.css (right): https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... chrome/browser/resources/history/history.css:83: float: right; Hmm, I don't see a corresponding float: left for these elements. https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... chrome/browser/resources/history/history.css:118: html[dir='ltr'] #range-next { ltr should never be specified. It is the default. https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... chrome/browser/resources/history/history.css:120: border-left: 1px solid rgba(0, 0, 0, 0.25); Only the left/right properties should be under the 'rtl' selector. https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... chrome/browser/resources/history/history.css:339: width: 68px; Setting width after min-width does nothing; remove it. https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... chrome/browser/resources/history/history.css:360: width: 90px; Same as above.
https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... File chrome/browser/resources/history/history.css (right): https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... chrome/browser/resources/history/history.css:83: float: right; On 2013/06/10 01:37:11, James Hawkins wrote: > Hmm, I don't see a corresponding float: left for these elements. I've put it explicitly now. https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... chrome/browser/resources/history/history.css:118: html[dir='ltr'] #range-next { On 2013/06/10 01:37:11, James Hawkins wrote: > ltr should never be specified. It is the default. In this case I have two buttons that have to be flipped in RTL. so if I don't specify dir='ltr' that then both #range-previous and html[dir='rtl'] #range-previous would apply when RTL. I've improved it by using more -webkit rules. https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... chrome/browser/resources/history/history.css:120: border-left: 1px solid rgba(0, 0, 0, 0.25); On 2013/06/10 01:37:11, James Hawkins wrote: > Only the left/right properties should be under the 'rtl' selector. Updated (but they contained only *-left rules before as well). https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... chrome/browser/resources/history/history.css:339: width: 68px; On 2013/06/10 01:37:11, James Hawkins wrote: > Setting width after min-width does nothing; remove it. Done. https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... chrome/browser/resources/history/history.css:360: width: 90px; On 2013/06/10 01:37:11, James Hawkins wrote: > Same as above. Done.
https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... File chrome/browser/resources/history/history.html (right): https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... chrome/browser/resources/history/history.html:64: <div id="page" class="page"> Is there ever only going to be one page? https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... chrome/browser/resources/history/history.html:74: <img src="../disclosure_triangle_small.png" I thought we usually put images in CSS? https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... chrome/browser/resources/history/history.js:89: this.hostFilteringBehavior = result.hostFilteringBehavior || This relies on ALLOW having a falsy value, right? I think I would be more comfortable if you'd explicitly test for an undefined value.
https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... File chrome/browser/resources/history/history.html (right): https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... chrome/browser/resources/history/history.html:64: <div id="page" class="page"> On 2013/06/10 12:33:27, Bernhard Bauer wrote: > Is there ever only going to be one page? Renamed to "history-page". I need to get that element somehow in an unique way so using the current class wouldn't work I think. https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... chrome/browser/resources/history/history.html:74: <img src="../disclosure_triangle_small.png" On 2013/06/10 12:33:27, Bernhard Bauer wrote: > I thought we usually put images in CSS? I remember discussing with Patrick (related CL where I started with the CSS approach and moved to <img>: https://codereview.chromium.org/12218058/) and deciding to go with images and alt. https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... chrome/browser/resources/history/history.js:89: this.hostFilteringBehavior = result.hostFilteringBehavior || On 2013/06/10 12:33:27, Bernhard Bauer wrote: > This relies on ALLOW having a falsy value, right? I think I would be more > comfortable if you'd explicitly test for an undefined value. Done.
https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... File chrome/browser/resources/history/history.html (right): https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... chrome/browser/resources/history/history.html:74: <img src="../disclosure_triangle_small.png" On 2013/06/10 13:18:41, Sergiu wrote: > On 2013/06/10 12:33:27, Bernhard Bauer wrote: > > I thought we usually put images in CSS? > > I remember discussing with Patrick (related CL where I started with the CSS > approach and moved to <img>: https://codereview.chromium.org/12218058/) and > deciding to go with images and alt. Not sure what you mean; Pat said you should use a background image (i.e. the CSS attribute) instead of image contents. https://codereview.chromium.org/15969014/diff/53002/chrome/browser/resources/... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/15969014/diff/53002/chrome/browser/resources/... chrome/browser/resources/history/history.js:93: this.hostFilteringBehavior = result.hostFilteringBehavior; Indent only two spaces.
https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... File chrome/browser/resources/history/history.html (right): https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... chrome/browser/resources/history/history.html:74: <img src="../disclosure_triangle_small.png" On 2013/06/10 13:30:28, Bernhard Bauer wrote: > On 2013/06/10 13:18:41, Sergiu wrote: > > On 2013/06/10 12:33:27, Bernhard Bauer wrote: > > > I thought we usually put images in CSS? > > > > I remember discussing with Patrick (related CL where I started with the CSS > > approach and moved to <img>: https://codereview.chromium.org/12218058/) and > > deciding to go with images and alt. > > Not sure what you mean; Pat said you should use a background image (i.e. the CSS > attribute) instead of image contents. Yes, at that time he was referring to using "background-image" instead of "content", but I remember I talked off-CL with him and we agreed on the image tag (which has the advantage of being accessible, since it can have alt text). In the other places we use "background-image" we're not really using the button tag. https://codereview.chromium.org/15969014/diff/53002/chrome/browser/resources/... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/15969014/diff/53002/chrome/browser/resources/... chrome/browser/resources/history/history.js:93: this.hostFilteringBehavior = result.hostFilteringBehavior; On 2013/06/10 13:30:28, Bernhard Bauer wrote: > Indent only two spaces. Done.
https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... File chrome/browser/resources/history/history.html (right): https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... chrome/browser/resources/history/history.html:74: <img src="../disclosure_triangle_small.png" On 2013/06/10 13:40:45, Sergiu wrote: > On 2013/06/10 13:30:28, Bernhard Bauer wrote: > > On 2013/06/10 13:18:41, Sergiu wrote: > > > On 2013/06/10 12:33:27, Bernhard Bauer wrote: > > > > I thought we usually put images in CSS? > > > > > > I remember discussing with Patrick (related CL where I started with the CSS > > > approach and moved to <img>: https://codereview.chromium.org/12218058/) and > > > deciding to go with images and alt. > > > > Not sure what you mean; Pat said you should use a background image (i.e. the > CSS > > attribute) instead of image contents. > > Yes, at that time he was referring to using "background-image" instead of > "content", but I remember I talked off-CL with him and we agreed on the image > tag (which has the advantage of being accessible, since it can have alt text). Hm. The CSS property has the advantage that you can specify HiDPI images with -webkit-image-set. Can't you set the alt text on the button instead? Alternatively, use the alt text as content for the button, then apply a style sheet that sets it to empty? > In the other places we use "background-image" we're not really using the button > tag. Well, the default stylesheet we use (widgets.css) uses a gradient and not an image background for buttons, but it does use the background-image property.
https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... File chrome/browser/resources/history/history.html (right): https://codereview.chromium.org/15969014/diff/36001/chrome/browser/resources/... chrome/browser/resources/history/history.html:74: <img src="../disclosure_triangle_small.png" On 2013/06/10 14:18:27, Bernhard Bauer wrote: > On 2013/06/10 13:40:45, Sergiu wrote: > > On 2013/06/10 13:30:28, Bernhard Bauer wrote: > > > On 2013/06/10 13:18:41, Sergiu wrote: > > > > On 2013/06/10 12:33:27, Bernhard Bauer wrote: > > > > > I thought we usually put images in CSS? > > > > > > > > I remember discussing with Patrick (related CL where I started with the > CSS > > > > approach and moved to <img>: https://codereview.chromium.org/12218058/) > and > > > > deciding to go with images and alt. > > > > > > Not sure what you mean; Pat said you should use a background image (i.e. the > > CSS > > > attribute) instead of image contents. > > > > Yes, at that time he was referring to using "background-image" instead of > > "content", but I remember I talked off-CL with him and we agreed on the image > > tag (which has the advantage of being accessible, since it can have alt text). > > Hm. The CSS property has the advantage that you can specify HiDPI images with > -webkit-image-set. Can't you set the alt text on the button instead? > Alternatively, use the alt text as content for the button, then apply a style > sheet that sets it to empty? > > > In the other places we use "background-image" we're not really using the > button > > tag. > > Well, the default stylesheet we use (widgets.css) uses a gradient and not an > image background for buttons, but it does use the background-image property. Done, see the new patch.
LGTM, just one nit: https://codereview.chromium.org/15969014/diff/64001/chrome/browser/resources/... File chrome/browser/resources/history/history.html (right): https://codereview.chromium.org/15969014/diff/64001/chrome/browser/resources/... chrome/browser/resources/history/history.html:74: </button><button id="range-next" i18n-values="alt:rangeNext" disabled> Can you put the closing tag on the previous line? If that doesn't fit, put the opening tag on a new line?
Whoops, I forgot about the browser tests, fixed that now as well.
LGTM still holds.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergiu@chromium.org/15969014/81001
James, I see you haven't commented on anything else. Since I'm pretty late on this CL I've checked the commit box, if you think anything else should be changed I'll address it in a new CL. Thanks!
LGTM with nit. https://codereview.chromium.org/15969014/diff/81001/chrome/browser/resources/... File chrome/browser/resources/history/history.css (right): https://codereview.chromium.org/15969014/diff/81001/chrome/browser/resources/... chrome/browser/resources/history/history.css:103: html[dir='rtl'] #range-today, nit: This should move below the non-RTL versions for the sake of coherency.
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergiu@chromium.org/15969014/81001
Message was sent while issue was closed.
Change committed as 205757 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
