|
|
Created:
7 years, 7 months ago by mtomasz Modified:
7 years, 7 months ago Reviewers:
yoshiki CC:
chromium-reviews, rginda+watch_chromium.org, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd window buttons (close and maximize) to the Files.app's new UI.
In the new UI decoration buttons have been removed, and were replaced by html ones. However, in gallery they were still missing. This patch fixes this issue by adding these two buttons.
TEST=Run Files.app, open a picture, try to maximize and close. Check if the back button also works. Check the legacy ui. Check the legacy Files.app.
BUG=236311
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=199387
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressed comments. #
Total comments: 16
Patch Set 3 : Addressed comments. #Patch Set 4 : Rebased #
Messages
Total messages: 15 (0 generated)
@yoshiki: PTAL. Thanks.
https://codereview.chromium.org/14759019/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/js/photo/gallery.js (right): https://codereview.chromium.org/14759019/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/photo/gallery.js:38: * {function} onBack s/function/function(string)/ https://codereview.chromium.org/14759019/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/photo/gallery.js:39: * {function} onClose s/function/function()/ https://codereview.chromium.org/14759019/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/photo/gallery.js:40: * {function} onMaximize ditto https://codereview.chromium.org/14759019/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/photo/gallery.js:127: var onBack = function() { I should think that a 'selectedUrls' parameter is passed as an argument. Shouldn't we use it? https://codereview.chromium.org/14759019/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/photo/gallery.js:129: document.location = 'main.html?' + Shouldn't we use 'main_new_ui.html' in new ui?
https://codereview.chromium.org/14759019/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/js/photo/gallery.js (right): https://codereview.chromium.org/14759019/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/photo/gallery.js:38: * {function} onBack On 2013/05/07 02:21:50, yoshiki wrote: > s/function/function(string)/ Done. https://codereview.chromium.org/14759019/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/photo/gallery.js:39: * {function} onClose On 2013/05/07 02:21:50, yoshiki wrote: > s/function/function()/ Done. https://codereview.chromium.org/14759019/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/photo/gallery.js:40: * {function} onMaximize On 2013/05/07 02:21:50, yoshiki wrote: > ditto Done. https://codereview.chromium.org/14759019/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/photo/gallery.js:127: var onBack = function() { On 2013/05/07 02:21:50, yoshiki wrote: > I should think that a 'selectedUrls' parameter is passed as an argument. > Shouldn't we use it? We get the selected path from the current location.hash. https://codereview.chromium.org/14759019/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/photo/gallery.js:129: document.location = 'main.html?' + On 2013/05/07 02:21:50, yoshiki wrote: > Shouldn't we use 'main_new_ui.html' in new ui? This seem not to be used in Files.app V2. I've added a comment to remove this code.
LGTM after some comments are addressed. https://codereview.chromium.org/14759019/diff/4001/chrome/browser/resources/f... File chrome/browser/resources/file_manager/css/common.css (right): https://codereview.chromium.org/14759019/diff/4001/chrome/browser/resources/f... chrome/browser/resources/file_manager/css/common.css:463: -webkit-linear-gradient(#f4f4f4, #efefef 40%, #dcdcdc); Use 'background-image' like other changes (L453, L472) if possible. https://codereview.chromium.org/14759019/diff/4001/chrome/browser/resources/f... File chrome/browser/resources/file_manager/css/gallery.css (right): https://codereview.chromium.org/14759019/diff/4001/chrome/browser/resources/f... chrome/browser/resources/file_manager/css/gallery.css:307: body[new-ui] .gallery[tools][editing] *[dimmed] * { Why do you specify both 'body[new-ui]' and 'body:not([new-ui])'? You can remove them as following. .gallery[tools][editing] *[dimmed], .gallery[tools][editing] *[dimmed] * { https://codereview.chromium.org/14759019/diff/4001/chrome/browser/resources/f... chrome/browser/resources/file_manager/css/gallery.css:312: body[new-ui] .gallery[tools][editing] *[dimmed] { ditto https://codereview.chromium.org/14759019/diff/4001/chrome/browser/resources/f... chrome/browser/resources/file_manager/css/gallery.css:614: min-width: 0; /* R. */ Unintentional change? https://codereview.chromium.org/14759019/diff/4001/chrome/browser/resources/f... chrome/browser/resources/file_manager/css/gallery.css:625: body .gallery button:hover { Remove 'body'. It should be unnecessary. https://codereview.chromium.org/14759019/diff/4001/chrome/browser/resources/f... chrome/browser/resources/file_manager/css/gallery.css:630: body .gallery button:active, ditt https://codereview.chromium.org/14759019/diff/4001/chrome/browser/resources/f... chrome/browser/resources/file_manager/css/gallery.css:631: body .gallery button[pressed], ditto https://codereview.chromium.org/14759019/diff/4001/chrome/browser/resources/f... chrome/browser/resources/file_manager/css/gallery.css:632: body .gallery button[pressed]:hover { ditto
https://codereview.chromium.org/14759019/diff/4001/chrome/browser/resources/f... File chrome/browser/resources/file_manager/css/gallery.css (right): https://codereview.chromium.org/14759019/diff/4001/chrome/browser/resources/f... chrome/browser/resources/file_manager/css/gallery.css:307: body[new-ui] .gallery[tools][editing] *[dimmed] * { On 2013/05/07 04:01:30, yoshiki wrote: > Why do you specify both 'body[new-ui]' and 'body:not([new-ui])'? You can remove > them as following. > > .gallery[tools][editing] *[dimmed], > .gallery[tools][editing] *[dimmed] * { Body is required to override original style. Eg. if we have: Eg. for: body[new-ui] A { opacity: 1.0 } A[dimmed] { opacity: 0.5 } will not override it, but body[new-ui] A[dimmed] { opacity: 0.5 } will.
https://codereview.chromium.org/14759019/diff/4001/chrome/browser/resources/f... File chrome/browser/resources/file_manager/css/gallery.css (right): https://codereview.chromium.org/14759019/diff/4001/chrome/browser/resources/f... chrome/browser/resources/file_manager/css/gallery.css:307: body[new-ui] .gallery[tools][editing] *[dimmed] * { On 2013/05/07 04:03:42, mtomasz wrote: > On 2013/05/07 04:01:30, yoshiki wrote: > > Why do you specify both 'body[new-ui]' and 'body:not([new-ui])'? You can > remove > > them as following. > > > > .gallery[tools][editing] *[dimmed], > > .gallery[tools][editing] *[dimmed] * { > > Body is required to override original style. Eg. if we have: > > Eg. for: > body[new-ui] A { opacity: 1.0 } > > A[dimmed] { opacity: 0.5 } will not override it, but > body[new-ui] A[dimmed] { opacity: 0.5 } will. Got it. Please add comments for the feature. Thanks.
https://codereview.chromium.org/14759019/diff/4001/chrome/browser/resources/f... File chrome/browser/resources/file_manager/css/common.css (right): https://codereview.chromium.org/14759019/diff/4001/chrome/browser/resources/f... chrome/browser/resources/file_manager/css/common.css:463: -webkit-linear-gradient(#f4f4f4, #efefef 40%, #dcdcdc); On 2013/05/07 04:01:30, yoshiki wrote: > Use 'background-image' like other changes (L453, L472) if possible. Done! https://codereview.chromium.org/14759019/diff/4001/chrome/browser/resources/f... File chrome/browser/resources/file_manager/css/gallery.css (right): https://codereview.chromium.org/14759019/diff/4001/chrome/browser/resources/f... chrome/browser/resources/file_manager/css/gallery.css:614: min-width: 0; /* R. */ On 2013/05/07 04:01:30, yoshiki wrote: > Unintentional change? Done. https://codereview.chromium.org/14759019/diff/4001/chrome/browser/resources/f... chrome/browser/resources/file_manager/css/gallery.css:625: body .gallery button:hover { On 2013/05/07 04:01:30, yoshiki wrote: > Remove 'body'. It should be unnecessary. We need it. Done. https://codereview.chromium.org/14759019/diff/4001/chrome/browser/resources/f... chrome/browser/resources/file_manager/css/gallery.css:630: body .gallery button:active, On 2013/05/07 04:01:30, yoshiki wrote: > ditt Done. https://codereview.chromium.org/14759019/diff/4001/chrome/browser/resources/f... chrome/browser/resources/file_manager/css/gallery.css:631: body .gallery button[pressed], On 2013/05/07 04:01:30, yoshiki wrote: > ditto Done. https://codereview.chromium.org/14759019/diff/4001/chrome/browser/resources/f... chrome/browser/resources/file_manager/css/gallery.css:632: body .gallery button[pressed]:hover { On 2013/05/07 04:01:30, yoshiki wrote: > ditto Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mtomasz@chromium.org/14759019/10001
Failed to apply patch for chrome/browser/resources/file_manager/css/gallery.css: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file chrome/browser/resources/file_manager/css/gallery.css Hunk #9 FAILED at 586. Hunk #10 FAILED at 615. 2 out of 14 hunks FAILED -- saving rejects to file chrome/browser/resources/file_manager/css/gallery.css.rej Patch: chrome/browser/resources/file_manager/css/gallery.css Index: chrome/browser/resources/file_manager/css/gallery.css diff --git a/chrome/browser/resources/file_manager/css/gallery.css b/chrome/browser/resources/file_manager/css/gallery.css index 71ac70614b4c89b9225dd258eb0a36c75a1ee362..9b94be807a9d1f2bb56343841575904223950b65 100644 --- a/chrome/browser/resources/file_manager/css/gallery.css +++ b/chrome/browser/resources/file_manager/css/gallery.css @@ -28,7 +28,7 @@ body { /* We actually want (left,top) to be (0,0) but for some weird reason this triggers :hover style on page reload which is ugly. */ -.gallery > .close { +.gallery > .back-button { cursor: pointer; left: 1px; position: absolute; @@ -36,14 +36,14 @@ body { z-index: 200; } -body[new-ui] .tools .gallery > .close { +body[new-ui] .tools .gallery > .back-button { opacity: 0; } /* The close icon is in a nested div so that its opacity can be manipulated independently from its parent (which can be dimmed when the crop frame overlaps it) */ -.gallery > .close div { +.gallery > .back-button div { background-image: -webkit-image-set( url('../images/gallery/back_to_files.png') 1x, url('../images/gallery/2x/back_to_files.png') 2x); @@ -54,11 +54,11 @@ body[new-ui] .tools .gallery > .close { width: 64px; } -.gallery[tools] > .close div { +.gallery[tools] > .back-button div { opacity: 0.5; } -.gallery[tools] > .close div:hover { +.gallery[tools] > .back-button div:hover { opacity: 1; } @@ -179,9 +179,8 @@ body[new-ui] .gallery > .header, background-color: rgba(18, 18, 18, 0.875); display: -webkit-box; left: 0; - min-width: 800px; opacity: 0; - padding: 0 8px; + padding: 0 10px; pointer-events: none; position: absolute; right: 0; @@ -193,7 +192,10 @@ body[new-ui] .gallery > .toolbar { } body[new-ui] .gallery > .header { + -webkit-box-align: center; + -webkit-box-pack: end; border-bottom: 1px solid rgba(50, 50, 50, 0.8); + display: -webkit-box; height: 45px; top: 0; } @@ -202,6 +204,7 @@ body[new-ui] .gallery > .header { border-top: 1px solid rgba(31, 31, 31, 0.7); bottom: 0; height: 55px; + min-width: 800px; } body[new-ui] .gallery > .toolbar { @@ -298,12 +301,15 @@ body[new-ui] .gallery[tools]:not([slideshow]) > .header, /* The editor marks elements with 'dimmed' attribute to get them out of the way of the crop frame */ -.gallery[tools][editing] *[dimmed], -.gallery[tools][editing] *[dimmed] * { +body:not([new-ui]) .gallery[tools][editing] *[dimmed], +body[new-ui] .gallery[tools][editing] *[dimmed], +body:not([new-ui]) .gallery[tools][editing] *[dimmed] *, +body[new-ui] .gallery[tools][editing] *[dimmed] * { pointer-events: none; } -.gallery[tools][editing] *[dimmed] { +body:not([new-ui]) .gallery[tools][editing] *[dimmed], +body[new-ui] .gallery[tools][editing] *[dimmed] { opacity: 0.2; } @@ -569,7 +575,8 @@ body[new-ui] .gallery .image-wrapper > img:not(.cached) { visibility: visible; } -body .gallery > .toolbar button { +body .gallery button, +body .gallery button[disabled] { -webkit-box-align: center; -webkit-box-flex: 0; -webkit-box-orient: horizontal; @@ -579,27 +586,29 @@ body .gallery > .toolbar button { background-position: center; background-repeat: no-repeat; border: none; - box-sizing: content-box; color: white; cursor: pointer; display: -webkit-box; - height: 40px; - margin: 8px 0 7px 3px; - min-width: 40px; /* Reset. */ opacity: 0.99; /* Workaround for http://crosbug.com/21065 */ - padding: 0; + padding: 1px; /* Instead of a border. */ position: relative; - width: 40px; z-index: 10; } +body .gallery button { + height: 40px; + margin: 6px 0; + min-width: 40px; /* Reset. */ + width: 40px; +} + /* By default, labels are hidden. */ .gallery > .toolbar button span { display: none; } /* Show labels if there is enough space. */ -@media (min-width: 1000px) { +@media (min-width: 1050px) { .gallery .edit-main button { background-position: 5px center; @@ -608,20 +617,20 @@ body .gallery > .toolbar button { width: auto; } - .gallery > .toolbar button span { + .gallery button span { display: inline; } } -.gallery > .toolbar button:hover { +body .gallery button:hover { background-color: rgba(31, 31, 31, 1); color: white; } -.gallery > .toolbar button:active, -.gallery > .toolbar button[pressed], -.gallery > .toolbar button[pressed]:hover { +body .gallery button:active, +body .gallery button[pressed], +body .gallery button[pressed]:hover { background-color: rgba(240, 240, 240, 1); color: black; } @@ -979,7 +988,7 @@ body .gallery > .toolbar button { -webkit-box-align: center; } -.gallery .prompt-wrapper[pos=center] .close { +.gallery .prompt-wrapper[pos=center] .back-button { display: none; } @@ -1017,7 +1026,7 @@ body .gallery > .toolbar button { padding-right: 10px; } -.gallery .prompt .close { +.gallery .prompt .back-button { background-image: -webkit-image-set( url('../images/gallery/butterbar_close_x.png') 1x, url('../images/gallery/2x/butterbar_close_x.png') 2x); @@ -1030,7 +1039,7 @@ body .gallery > .toolbar button { width: 16px; } -.gallery .prompt .close:hover { +.gallery .prompt .back-button:hover { background-color: rgba(81, 81, 81, 1); opacity: 1.0; } @@ -1345,3 +1354,21 @@ body .gallery > .toolbar button { url('../images/gallery/2x/slideshow-end.png') 2x); margin-left: -2px; } + +body[new-ui] .gallery > .header > button { + height: 27px; + min-width: 29px; + width: 29px; +} + +body[new-ui] .gallery > .header > .maximize-button { + background-image: -webkit-image-set( + url('../images/files/ui/new-ui/topbar_button_maximize.png') 1x, + url('../images/files/ui/new-ui/2x/topbar_button_maximize.png') 2x); +} + +body[new-ui] .gallery > .header > .close-button { + background-image: -webkit-image-set( + url('../images/files/ui/new-ui/topbar_button_close.png') 1x, + url('../images/files/ui/new-ui/2x/topbar_button_close.png') 2x); +}
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mtomasz@chromium.org/14759019/10001
Failed to apply patch for chrome/browser/resources/file_manager/css/gallery.css: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file chrome/browser/resources/file_manager/css/gallery.css Hunk #9 FAILED at 586. Hunk #10 FAILED at 615. 2 out of 14 hunks FAILED -- saving rejects to file chrome/browser/resources/file_manager/css/gallery.css.rej Patch: chrome/browser/resources/file_manager/css/gallery.css Index: chrome/browser/resources/file_manager/css/gallery.css diff --git a/chrome/browser/resources/file_manager/css/gallery.css b/chrome/browser/resources/file_manager/css/gallery.css index 71ac70614b4c89b9225dd258eb0a36c75a1ee362..9b94be807a9d1f2bb56343841575904223950b65 100644 --- a/chrome/browser/resources/file_manager/css/gallery.css +++ b/chrome/browser/resources/file_manager/css/gallery.css @@ -28,7 +28,7 @@ body { /* We actually want (left,top) to be (0,0) but for some weird reason this triggers :hover style on page reload which is ugly. */ -.gallery > .close { +.gallery > .back-button { cursor: pointer; left: 1px; position: absolute; @@ -36,14 +36,14 @@ body { z-index: 200; } -body[new-ui] .tools .gallery > .close { +body[new-ui] .tools .gallery > .back-button { opacity: 0; } /* The close icon is in a nested div so that its opacity can be manipulated independently from its parent (which can be dimmed when the crop frame overlaps it) */ -.gallery > .close div { +.gallery > .back-button div { background-image: -webkit-image-set( url('../images/gallery/back_to_files.png') 1x, url('../images/gallery/2x/back_to_files.png') 2x); @@ -54,11 +54,11 @@ body[new-ui] .tools .gallery > .close { width: 64px; } -.gallery[tools] > .close div { +.gallery[tools] > .back-button div { opacity: 0.5; } -.gallery[tools] > .close div:hover { +.gallery[tools] > .back-button div:hover { opacity: 1; } @@ -179,9 +179,8 @@ body[new-ui] .gallery > .header, background-color: rgba(18, 18, 18, 0.875); display: -webkit-box; left: 0; - min-width: 800px; opacity: 0; - padding: 0 8px; + padding: 0 10px; pointer-events: none; position: absolute; right: 0; @@ -193,7 +192,10 @@ body[new-ui] .gallery > .toolbar { } body[new-ui] .gallery > .header { + -webkit-box-align: center; + -webkit-box-pack: end; border-bottom: 1px solid rgba(50, 50, 50, 0.8); + display: -webkit-box; height: 45px; top: 0; } @@ -202,6 +204,7 @@ body[new-ui] .gallery > .header { border-top: 1px solid rgba(31, 31, 31, 0.7); bottom: 0; height: 55px; + min-width: 800px; } body[new-ui] .gallery > .toolbar { @@ -298,12 +301,15 @@ body[new-ui] .gallery[tools]:not([slideshow]) > .header, /* The editor marks elements with 'dimmed' attribute to get them out of the way of the crop frame */ -.gallery[tools][editing] *[dimmed], -.gallery[tools][editing] *[dimmed] * { +body:not([new-ui]) .gallery[tools][editing] *[dimmed], +body[new-ui] .gallery[tools][editing] *[dimmed], +body:not([new-ui]) .gallery[tools][editing] *[dimmed] *, +body[new-ui] .gallery[tools][editing] *[dimmed] * { pointer-events: none; } -.gallery[tools][editing] *[dimmed] { +body:not([new-ui]) .gallery[tools][editing] *[dimmed], +body[new-ui] .gallery[tools][editing] *[dimmed] { opacity: 0.2; } @@ -569,7 +575,8 @@ body[new-ui] .gallery .image-wrapper > img:not(.cached) { visibility: visible; } -body .gallery > .toolbar button { +body .gallery button, +body .gallery button[disabled] { -webkit-box-align: center; -webkit-box-flex: 0; -webkit-box-orient: horizontal; @@ -579,27 +586,29 @@ body .gallery > .toolbar button { background-position: center; background-repeat: no-repeat; border: none; - box-sizing: content-box; color: white; cursor: pointer; display: -webkit-box; - height: 40px; - margin: 8px 0 7px 3px; - min-width: 40px; /* Reset. */ opacity: 0.99; /* Workaround for http://crosbug.com/21065 */ - padding: 0; + padding: 1px; /* Instead of a border. */ position: relative; - width: 40px; z-index: 10; } +body .gallery button { + height: 40px; + margin: 6px 0; + min-width: 40px; /* Reset. */ + width: 40px; +} + /* By default, labels are hidden. */ .gallery > .toolbar button span { display: none; } /* Show labels if there is enough space. */ -@media (min-width: 1000px) { +@media (min-width: 1050px) { .gallery .edit-main button { background-position: 5px center; @@ -608,20 +617,20 @@ body .gallery > .toolbar button { width: auto; } - .gallery > .toolbar button span { + .gallery button span { display: inline; } } -.gallery > .toolbar button:hover { +body .gallery button:hover { background-color: rgba(31, 31, 31, 1); color: white; } -.gallery > .toolbar button:active, -.gallery > .toolbar button[pressed], -.gallery > .toolbar button[pressed]:hover { +body .gallery button:active, +body .gallery button[pressed], +body .gallery button[pressed]:hover { background-color: rgba(240, 240, 240, 1); color: black; } @@ -979,7 +988,7 @@ body .gallery > .toolbar button { -webkit-box-align: center; } -.gallery .prompt-wrapper[pos=center] .close { +.gallery .prompt-wrapper[pos=center] .back-button { display: none; } @@ -1017,7 +1026,7 @@ body .gallery > .toolbar button { padding-right: 10px; } -.gallery .prompt .close { +.gallery .prompt .back-button { background-image: -webkit-image-set( url('../images/gallery/butterbar_close_x.png') 1x, url('../images/gallery/2x/butterbar_close_x.png') 2x); @@ -1030,7 +1039,7 @@ body .gallery > .toolbar button { width: 16px; } -.gallery .prompt .close:hover { +.gallery .prompt .back-button:hover { background-color: rgba(81, 81, 81, 1); opacity: 1.0; } @@ -1345,3 +1354,21 @@ body .gallery > .toolbar button { url('../images/gallery/2x/slideshow-end.png') 2x); margin-left: -2px; } + +body[new-ui] .gallery > .header > button { + height: 27px; + min-width: 29px; + width: 29px; +} + +body[new-ui] .gallery > .header > .maximize-button { + background-image: -webkit-image-set( + url('../images/files/ui/new-ui/topbar_button_maximize.png') 1x, + url('../images/files/ui/new-ui/2x/topbar_button_maximize.png') 2x); +} + +body[new-ui] .gallery > .header > .close-button { + background-image: -webkit-image-set( + url('../images/files/ui/new-ui/topbar_button_close.png') 1x, + url('../images/files/ui/new-ui/2x/topbar_button_close.png') 2x); +}
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mtomasz@chromium.org/14759019/25001
Retried try job too often on win7_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mtomasz@chromium.org/14759019/25001
Message was sent while issue was closed.
Change committed as 199387 |