Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2376163002:
[Blink] Display images in the center of the screen (Closed)
Description[Blink] Display images in the center of the screen
Code was adapted from MediaDocument.cpp. Additions were
made to let the image be zoomed to full width on both
mobile and desktop.
Had to edit TestExpectations to deal with the virtual tests
failing to work with the command line rebaselining tool,
but some of the suggested changes are included here.
Intent to implement:
https://docs.google.com/document/d/1wJPz4kn-9mEFs_ZEM0hAwwr-YzBdW-5hdX1-jkt81Ng/edit?usp=sharing
Images of proposed changes:
https://drive.google.com/drive/folders/0B8k2SmEN8xS3RHJGSVpNZjVtUUE?usp=sharing
BUG=650456
Committed: https://crrev.com/8512a83031e6531c976f4af85a28dbeef58bd635
Cr-Commit-Position: refs/heads/master@{#425440}
Patch Set 1 #Patch Set 2 : Rebased #Patch Set 3 : Test rebaseline, part 1 #Patch Set 4 : [Blink] Display images on a black background in the center of the screen #Patch Set 5 : Wipe out test expectation changes #Patch Set 6 : Rebasing #Patch Set 7 : Fix SVG test #Patch Set 8 : Test expectations #Patch Set 9 : Virtual tests refusing to work with rebaseline tool #Patch Set 10 : Yet another attempt at rebaselining #
Total comments: 4
Patch Set 11 : Fixed viewport rendering of large images #Patch Set 12 : Rebased #Patch Set 13 : Checkerboard example #Patch Set 14 : Centering only #Patch Set 15 : Rebase #
Total comments: 6
Patch Set 16 : Comments #Patch Set 17 : Rebasing #Messages
Total messages: 53 (30 generated)
The CQ bit was checked by dfalcantara@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Blink] Display images on a black background in the center of the screen Display images in a manner similarly to how media files are displayed, centered on black. Additions were made to let the image be zoomed to full width on both mobile and desktop. Intent to implement: https://docs.google.com/document/d/1wJPz4kn-9mEFs_ZEM0hAwwr-YzBdW-5hdX1-jkt81... BUG=650456,650455 ========== to ========== [Blink] Display images on a black background in the center of the screen Display images in a manner similarly to how media files are displayed, centered on black. Additions were made to let the image be zoomed to full width on both mobile and desktop. Intent to implement: https://docs.google.com/document/d/1wJPz4kn-9mEFs_ZEM0hAwwr-YzBdW-5hdX1-jkt81... Video of Android behavior showing how a small image, a wide image, and a tall image are displayed: https://drive.google.com/open?id=0B8k2SmEN8xS3eHFMd0dZVDBFYkk Video of desktop behavior showing how a small image, a wide image, and a tall image are displayed: https://drive.google.com/file/d/0B8k2SmEN8xS3cTRLRTZqVjFLZ28/view BUG=650456,650455 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by dfalcantara@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Blink] Display images on a black background in the center of the screen Display images in a manner similarly to how media files are displayed, centered on black. Additions were made to let the image be zoomed to full width on both mobile and desktop. Intent to implement: https://docs.google.com/document/d/1wJPz4kn-9mEFs_ZEM0hAwwr-YzBdW-5hdX1-jkt81... Video of Android behavior showing how a small image, a wide image, and a tall image are displayed: https://drive.google.com/open?id=0B8k2SmEN8xS3eHFMd0dZVDBFYkk Video of desktop behavior showing how a small image, a wide image, and a tall image are displayed: https://drive.google.com/file/d/0B8k2SmEN8xS3cTRLRTZqVjFLZ28/view BUG=650456,650455 ========== to ========== [Blink] Display images on a black background in the center of the screen Display images in a manner similarly to how media files are displayed: centered on black. Code was adapted from MediaDocument.cpp. Additions were made to let the image be zoomed to full width on both mobile and desktop. Intent to implement: https://docs.google.com/document/d/1wJPz4kn-9mEFs_ZEM0hAwwr-YzBdW-5hdX1-jkt81... Video of Android behavior showing how a small image, a wide image, and a tall image are displayed: https://drive.google.com/open?id=0B8k2SmEN8xS3eHFMd0dZVDBFYkk Video of desktop behavior showing how a small image, a wide image, and a tall image are displayed: https://drive.google.com/file/d/0B8k2SmEN8xS3cTRLRTZqVjFLZ28/view BUG=650456,650455 ==========
Description was changed from ========== [Blink] Display images on a black background in the center of the screen Display images in a manner similarly to how media files are displayed, centered on black. Additions were made to let the image be zoomed to full width on both mobile and desktop. Intent to implement: https://docs.google.com/document/d/1wJPz4kn-9mEFs_ZEM0hAwwr-YzBdW-5hdX1-jkt81... Video of Android behavior showing how a small image, a wide image, and a tall image are displayed: https://drive.google.com/open?id=0B8k2SmEN8xS3eHFMd0dZVDBFYkk Video of desktop behavior showing how a small image, a wide image, and a tall image are displayed: https://drive.google.com/file/d/0B8k2SmEN8xS3cTRLRTZqVjFLZ28/view BUG=650456,650455 ========== to ========== [Blink] Display images on a black background in the center of the screen Display images in a manner similarly to how media files are displayed: centered on black. Code was adapted from MediaDocument.cpp. Additions were made to let the image be zoomed to full width on both mobile and desktop. Intent to implement: https://docs.google.com/document/d/1wJPz4kn-9mEFs_ZEM0hAwwr-YzBdW-5hdX1-jkt81... Video of Android behavior showing how a small image, a wide image, and a tall image are displayed: https://drive.google.com/open?id=0B8k2SmEN8xS3eHFMd0dZVDBFYkk Video of desktop behavior showing how a small image, a wide image, and a tall image are displayed: https://drive.google.com/file/d/0B8k2SmEN8xS3cTRLRTZqVjFLZ28/view BUG=650456,650455 ==========
The CQ bit was checked by dfalcantara@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Blink] Display images on a black background in the center of the screen Display images in a manner similarly to how media files are displayed: centered on black. Code was adapted from MediaDocument.cpp. Additions were made to let the image be zoomed to full width on both mobile and desktop. Intent to implement: https://docs.google.com/document/d/1wJPz4kn-9mEFs_ZEM0hAwwr-YzBdW-5hdX1-jkt81... Video of Android behavior showing how a small image, a wide image, and a tall image are displayed: https://drive.google.com/open?id=0B8k2SmEN8xS3eHFMd0dZVDBFYkk Video of desktop behavior showing how a small image, a wide image, and a tall image are displayed: https://drive.google.com/file/d/0B8k2SmEN8xS3cTRLRTZqVjFLZ28/view BUG=650456,650455 ========== to ========== [Blink] Display images on a black background in the center of the screen Display images in a manner similarly to how media files are displayed: centered on black. Code was adapted from MediaDocument.cpp. Additions were made to let the image be zoomed to full width on both mobile and desktop. Had to edit TestExpectations to deal with the virtual tests failing to work with the command line rebaselining tool, but some of the suggested changes are include here. Intent to implement: https://docs.google.com/document/d/1wJPz4kn-9mEFs_ZEM0hAwwr-YzBdW-5hdX1-jkt81... Video of Android behavior showing how a small image, a wide image, and a tall image are displayed: https://drive.google.com/open?id=0B8k2SmEN8xS3eHFMd0dZVDBFYkk Video of desktop behavior showing how a small image, a wide image, and a tall image are displayed: https://drive.google.com/file/d/0B8k2SmEN8xS3cTRLRTZqVjFLZ28/view BUG=650456,650455 ==========
Description was changed from ========== [Blink] Display images on a black background in the center of the screen Display images in a manner similarly to how media files are displayed: centered on black. Code was adapted from MediaDocument.cpp. Additions were made to let the image be zoomed to full width on both mobile and desktop. Had to edit TestExpectations to deal with the virtual tests failing to work with the command line rebaselining tool, but some of the suggested changes are include here. Intent to implement: https://docs.google.com/document/d/1wJPz4kn-9mEFs_ZEM0hAwwr-YzBdW-5hdX1-jkt81... Video of Android behavior showing how a small image, a wide image, and a tall image are displayed: https://drive.google.com/open?id=0B8k2SmEN8xS3eHFMd0dZVDBFYkk Video of desktop behavior showing how a small image, a wide image, and a tall image are displayed: https://drive.google.com/file/d/0B8k2SmEN8xS3cTRLRTZqVjFLZ28/view BUG=650456,650455 ========== to ========== [Blink] Display images on a black background in the center of the screen Display images in a manner similarly to how media files are displayed: centered on black. Code was adapted from MediaDocument.cpp. Additions were made to let the image be zoomed to full width on both mobile and desktop. Had to edit TestExpectations to deal with the virtual tests failing to work with the command line rebaselining tool, but some of the suggested changes are included here. Intent to implement: https://docs.google.com/document/d/1wJPz4kn-9mEFs_ZEM0hAwwr-YzBdW-5hdX1-jkt81... Video of Android behavior showing how a small image, a wide image, and a tall image are displayed: https://drive.google.com/open?id=0B8k2SmEN8xS3eHFMd0dZVDBFYkk Video of desktop behavior showing how a small image, a wide image, and a tall image are displayed: https://drive.google.com/file/d/0B8k2SmEN8xS3cTRLRTZqVjFLZ28/view BUG=650456,650455 ==========
Hey pdr@, do you know who should be reviewing this? Saw you'd been listed as a reviewer for an ImageDocument CL about a month ago, figured you'd at least be able to help triage. Thanks!
pdr@chromium.org changed reviewers: + esprehn@chromium.org, pdr@chromium.org
On 2016/10/07 at 21:59:28, dfalcantara wrote: > Hey pdr@, do you know who should be reviewing this? Saw you'd been listed as a reviewer for an ImageDocument CL about a month ago, figured you'd at least be able to help triage. Thanks! I'm not sure there is a good reviewer for this. Myself + Esprehn would probably be reasonable. @Elliott, can you review this as well? Overall LGTM. One nit: in the Android example, why do we allow the user to pan down to all-black instead of forcing them to pan in the bounds of the image?
On 2016/10/07 22:12:28, pdr. wrote: > On 2016/10/07 at 21:59:28, dfalcantara wrote: > > Hey pdr@, do you know who should be reviewing this? Saw you'd been listed as > a reviewer for an ImageDocument CL about a month ago, figured you'd at least be > able to help triage. Thanks! > > I'm not sure there is a good reviewer for this. Myself + Esprehn would probably > be reasonable. @Elliott, can you review this as well? > > Overall LGTM. > > One nit: in the Android example, why do we allow the user to pan down to > all-black instead of forcing them to pan in the bounds of the image? Thanks! I think the panning thing is a pre-existing behavior for how we show images; this video shows what currently happens: https://drive.google.com/open?id=0B8k2SmEN8xS3a21hdnJnTDRRNXM
On 2016/10/07 at 22:40:14, dfalcantara wrote: > On 2016/10/07 22:12:28, pdr. wrote: > > On 2016/10/07 at 21:59:28, dfalcantara wrote: > > > Hey pdr@, do you know who should be reviewing this? Saw you'd been listed as > > a reviewer for an ImageDocument CL about a month ago, figured you'd at least be > > able to help triage. Thanks! > > > > I'm not sure there is a good reviewer for this. Myself + Esprehn would probably > > be reasonable. @Elliott, can you review this as well? > > > > Overall LGTM. > > > > One nit: in the Android example, why do we allow the user to pan down to > > all-black instead of forcing them to pan in the bounds of the image? > > Thanks! > > I think the panning thing is a pre-existing behavior for how we show images; this video shows what currently happens: > https://drive.google.com/open?id=0B8k2SmEN8xS3a21hdnJnTDRRNXM I think we could further improve this by disallowing pan&zoom in the margin (like google photos). Still, this is a good change as-is. LGTM here! Please do wait for Elliott's final LGTM.
lgtm, thanks for being a product excellence hero! This is an awesome change. https://codereview.chromium.org/2376163002/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/custom/anchor-on-use-expected.svg (right): https://codereview.chromium.org/2376163002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/custom/anchor-on-use-expected.svg:2: <rect x="0" y="0" width="800" height="600" fill="black" /> why does this test change? https://codereview.chromium.org/2376163002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/ImageDocument.cpp (left): https://codereview.chromium.org/2376163002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/ImageDocument.cpp:374: // reading mode for normal-width-huge-height images. Why don't we need this now?
https://chromiumcodereview.appspot.com/2376163002/diff/180001/third_party/Web... File third_party/WebKit/LayoutTests/svg/custom/anchor-on-use-expected.svg (right): https://chromiumcodereview.appspot.com/2376163002/diff/180001/third_party/Web... third_party/WebKit/LayoutTests/svg/custom/anchor-on-use-expected.svg:2: <rect x="0" y="0" width="800" height="600" fill="black" /> On 2016/10/08 03:09:19, esprehn wrote: > why does this test change? The SVG showed the green checker image hugging the top-left corner on the default white background; I needed the SVG to reflect that it was centered on top of an 800x600 black rectangle — the test seems to use that size window. https://chromiumcodereview.appspot.com/2376163002/diff/180001/third_party/Web... File third_party/WebKit/Source/core/html/ImageDocument.cpp (left): https://chromiumcodereview.appspot.com/2376163002/diff/180001/third_party/Web... third_party/WebKit/Source/core/html/ImageDocument.cpp:374: // reading mode for normal-width-huge-height images. On 2016/10/08 03:09:19, esprehn wrote: > Why don't we need this now? Turns out we do... I tested a 16k x 1k image this morning (goo.gl/qPYwHI) and found that the patch doesn't allow zooming in as much as we used to. The main issue is that the original code allowed the image to be 10x the viewport's width, which didn't matter because it was hugging the top-left of the screen and no one would notice. With my patch, however, the max-width of the image is capped to the viewport's width, which means you can't zoom into the image as closely. I've been flailing about with various tweaks to the HTML, but usually the height of the parent div is set incorrectly, resulting in the image being vertically centered inside the div at the top of the page. Any advice on how to tweak the DOM for this case would be welcome.
On 2016/10/10 22:01:01, dfalcantara wrote: > https://chromiumcodereview.appspot.com/2376163002/diff/180001/third_party/Web... > File third_party/WebKit/LayoutTests/svg/custom/anchor-on-use-expected.svg > (right): > > https://chromiumcodereview.appspot.com/2376163002/diff/180001/third_party/Web... > third_party/WebKit/LayoutTests/svg/custom/anchor-on-use-expected.svg:2: <rect > x="0" y="0" width="800" height="600" fill="black" /> > On 2016/10/08 03:09:19, esprehn wrote: > > why does this test change? > > The SVG showed the green checker image hugging the top-left corner on the > default white background; I needed the SVG to reflect that it was centered on > top of an 800x600 black rectangle — the test seems to use that size window. > > https://chromiumcodereview.appspot.com/2376163002/diff/180001/third_party/Web... > File third_party/WebKit/Source/core/html/ImageDocument.cpp (left): > > https://chromiumcodereview.appspot.com/2376163002/diff/180001/third_party/Web... > third_party/WebKit/Source/core/html/ImageDocument.cpp:374: // reading mode for > normal-width-huge-height images. > On 2016/10/08 03:09:19, esprehn wrote: > > Why don't we need this now? > > Turns out we do... I tested a 16k x 1k image this morning (goo.gl/qPYwHI) and > found that the patch doesn't allow zooming in as much as we used to. The main > issue is that the original code allowed the image to be 10x the viewport's > width, which didn't matter because it was hugging the top-left of the screen and > no one would notice. With my patch, however, the max-width of the image is > capped to the viewport's width, which means you can't zoom into the image as > closely. > > I've been flailing about with various tweaks to the HTML, but usually the height > of the parent div is set incorrectly, resulting in the image being vertically > centered inside the div at the top of the page. Any advice on how to tweak the > DOM for this case would be welcome. I think I've got an idea of what to do about the case, which'll involve editing the style of the div to explicitly use the image and viewport sizes, effectively replacing that block I'd taken out. I'll hopefully have something to look at soon. Thanks!
The CQ bit was checked by dfalcantara@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Think I got it figured out; had to manually change the size of the containing div to properly center the image when it was super wide. PTAL when you get a chance.
On 2016/10/12 at 07:06:12, dfalcantara wrote: > Think I got it figured out; had to manually change the size of the containing div to properly center the image when it was super wide. PTAL when you get a chance. In testing this I found images with black (e.g., text) on transparency is kinda unreadable. I wonder if there's any way we could handle that? I don't have a good feeling for how much of a problem that will be; maybe it's not a big deal. I tested tall and wide images with and without device emulation in devtools and things look pretty good. LGTM there.
On 2016/10/12 20:21:40, pdr. wrote: > On 2016/10/12 at 07:06:12, dfalcantara wrote: > > Think I got it figured out; had to manually change the size of the containing > div to properly center the image when it was super wide. PTAL when you get a > chance. > > In testing this I found images with black (e.g., text) on transparency is kinda > unreadable. I wonder if there's any way we could handle that? I don't have a > good feeling for how much of a problem that will be; maybe it's not a big deal. > > I tested tall and wide images with and without device emulation in devtools and > things look pretty good. LGTM there. Par for the course, it seems (screenshots: https://goo.gl/8abHBc): * Photos & Hangouts show the image on black and make the black text invisible * Keep & Google+ show the image on white and makes the white text invisible The actual image: https://inadequatelycaffeinated.appspot.com/clank/download_tests/text_color.png UX said they were open to not using black, but that's how they've currently specced it. I personally think gray would make more sense, but we'd be inconsistent with videos.
Are videos ever transparent? Id suggest we make the IMG element itself have a grey background so for most things the page is black, but for transparent images you'd see grey in the gaps. For a largely transparent image you'd see the grey box, which is better than today where you see a blank page. -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Are videos ever transparent? Id suggest we make the IMG element itself have a grey background so for most things the page is black, but for transparent images you'd see grey in the gaps. For a largely transparent image you'd see the grey box, which is better than today where you see a blank page. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/10/12 21:04:51, esprehn wrote: > Are videos ever transparent? Id suggest we make the IMG element itself have > a grey background so for most things the page is black, but for transparent > images you'd see grey in the gaps. For a largely transparent image you'd > see the grey box, which is better than today where you see a blank page. It looks a little jarring for images that aren't mostly transparent, which I'm admittedly assuming is the more common use case: https://drive.google.com/open?id=0B8k2SmEN8xS3M3c5UkhYM1lrWGc
On 2016/10/12 21:25:20, dfalcantara wrote: > On 2016/10/12 21:04:51, esprehn wrote: > > Are videos ever transparent? Id suggest we make the IMG element itself have > > a grey background so for most things the page is black, but for transparent > > images you'd see grey in the gaps. For a largely transparent image you'd > > see the grey box, which is better than today where you see a blank page. > > It looks a little jarring for images that aren't mostly transparent, which > I'm admittedly assuming is the more common use case: > https://drive.google.com/open?id=0B8k2SmEN8xS3M3c5UkhYM1lrWGc Added another set of screenshots to that directory that uses wonky CSS to draw a white and gray checkerboard background behind the image, kind of like how Ubuntu's default image viewers does it. Thoughts? Do either of you feel strongly enough about this to poke UX before landing this CL?
On 2016/10/12 at 22:05:08, dfalcantara wrote: > On 2016/10/12 21:25:20, dfalcantara wrote: > > On 2016/10/12 21:04:51, esprehn wrote: > > > Are videos ever transparent? Id suggest we make the IMG element itself have > > > a grey background so for most things the page is black, but for transparent > > > images you'd see grey in the gaps. For a largely transparent image you'd > > > see the grey box, which is better than today where you see a blank page. > > > > It looks a little jarring for images that aren't mostly transparent, which > > I'm admittedly assuming is the more common use case: > > https://drive.google.com/open?id=0B8k2SmEN8xS3M3c5UkhYM1lrWGc > > Added another set of screenshots to that directory that uses wonky CSS to draw > a white and gray checkerboard background behind the image, kind of like how > Ubuntu's default image viewers does it. Thoughts? Do either of you feel > strongly enough about this to poke UX before landing this CL? I think the checkerboard look pretty good! I prefer the large checkers you've used, though the small checkers (like photoshop) are an option. I think it would be worth sending chrome UX an FYI email, just in case, but continue to land the patch. QQ: what does the incremental load story feel like? Does it start white, flash all black, draw a checkered square, then draw a white square, then line-by-line render in the image over the square? If this isn't perfect, I'd support fixing it in a followup instead of blocking this patch. Implementation-wise, I wonder if you can set the background of the image to be a css gradient of a checkerboard?
On 2016/10/12 22:26:36, pdr. wrote: > On 2016/10/12 at 22:05:08, dfalcantara wrote: > > On 2016/10/12 21:25:20, dfalcantara wrote: > > > On 2016/10/12 21:04:51, esprehn wrote: > > > > Are videos ever transparent? Id suggest we make the IMG element itself > have > > > > a grey background so for most things the page is black, but for > transparent > > > > images you'd see grey in the gaps. For a largely transparent image you'd > > > > see the grey box, which is better than today where you see a blank page. > > > > > > It looks a little jarring for images that aren't mostly transparent, which > > > I'm admittedly assuming is the more common use case: > > > https://drive.google.com/open?id=0B8k2SmEN8xS3M3c5UkhYM1lrWGc > > > > Added another set of screenshots to that directory that uses wonky CSS to draw > > a white and gray checkerboard background behind the image, kind of like how > > Ubuntu's default image viewers does it. Thoughts? Do either of you feel > > strongly enough about this to poke UX before landing this CL? > > I think the checkerboard look pretty good! I prefer the large checkers you've > used, though the small checkers (like photoshop) are an option. I think it would > be worth sending chrome UX an FYI email, just in case, but continue to land the > patch. Kicked off a thread with you folks on it. We'll see how it goes... > QQ: what does the incremental load story feel like? Does it start white, flash > all black, draw a checkered square, then draw a white square, then line-by-line > render in the image over the square? If this isn't perfect, I'd support fixing > it in a followup instead of blocking this patch. > > Implementation-wise, I wonder if you can set the background of the image to be a > css gradient of a checkerboard? Seems to do something close, and in my current hack (shown in PS13) it shows up behind all images: https://drive.google.com/open?id=0B8k2SmEN8xS3VE1uZjJ4Vmp3MG8
On 2016/10/13 00:31:53, dfalcantara wrote: > On 2016/10/12 22:26:36, pdr. wrote: > > On 2016/10/12 at 22:05:08, dfalcantara wrote: > > > On 2016/10/12 21:25:20, dfalcantara wrote: > > > > On 2016/10/12 21:04:51, esprehn wrote: > > > > > Are videos ever transparent? Id suggest we make the IMG element itself > > have > > > > > a grey background so for most things the page is black, but for > > transparent > > > > > images you'd see grey in the gaps. For a largely transparent image you'd > > > > > see the grey box, which is better than today where you see a blank page. > > > > > > > > It looks a little jarring for images that aren't mostly transparent, which > > > > I'm admittedly assuming is the more common use case: > > > > https://drive.google.com/open?id=0B8k2SmEN8xS3M3c5UkhYM1lrWGc > > > > > > Added another set of screenshots to that directory that uses wonky CSS to > draw > > > a white and gray checkerboard background behind the image, kind of like how > > > Ubuntu's default image viewers does it. Thoughts? Do either of you feel > > > strongly enough about this to poke UX before landing this CL? > > > > I think the checkerboard look pretty good! I prefer the large checkers you've > > used, though the small checkers (like photoshop) are an option. I think it > would > > be worth sending chrome UX an FYI email, just in case, but continue to land > the > > patch. > Kicked off a thread with you folks on it. We'll see how it goes... > > > QQ: what does the incremental load story feel like? Does it start white, flash > > all black, draw a checkered square, then draw a white square, then > line-by-line > > render in the image over the square? If this isn't perfect, I'd support fixing > > it in a followup instead of blocking this patch. > > > > Implementation-wise, I wonder if you can set the background of the image to be > a > > css gradient of a checkerboard? > > Seems to do something close, and in my current hack (shown in PS13) it shows up > behind all images: > https://drive.google.com/open?id=0B8k2SmEN8xS3VE1uZjJ4Vmp3MG8 I'm expecting show churn on that UX thread. Would either of you object to landing the bit that centers the image on screen while discussion about the background color continues?
On 2016/10/13 at 18:39:57, dfalcantara wrote: > I'm expecting show churn on that UX thread. Would either of you object to landing the bit that centers the image on screen while discussion about the background color continues? No objections here
Description was changed from ========== [Blink] Display images on a black background in the center of the screen Display images in a manner similarly to how media files are displayed: centered on black. Code was adapted from MediaDocument.cpp. Additions were made to let the image be zoomed to full width on both mobile and desktop. Had to edit TestExpectations to deal with the virtual tests failing to work with the command line rebaselining tool, but some of the suggested changes are included here. Intent to implement: https://docs.google.com/document/d/1wJPz4kn-9mEFs_ZEM0hAwwr-YzBdW-5hdX1-jkt81... Video of Android behavior showing how a small image, a wide image, and a tall image are displayed: https://drive.google.com/open?id=0B8k2SmEN8xS3eHFMd0dZVDBFYkk Video of desktop behavior showing how a small image, a wide image, and a tall image are displayed: https://drive.google.com/file/d/0B8k2SmEN8xS3cTRLRTZqVjFLZ28/view BUG=650456,650455 ========== to ========== [Blink] Display images in the center of the screen Code was adapted from MediaDocument.cpp. Additions were made to let the image be zoomed to full width on both mobile and desktop. Had to edit TestExpectations to deal with the virtual tests failing to work with the command line rebaselining tool, but some of the suggested changes are included here. Intent to implement: https://docs.google.com/document/d/1wJPz4kn-9mEFs_ZEM0hAwwr-YzBdW-5hdX1-jkt81... Images of proposed changes: https://drive.google.com/drive/folders/0B8k2SmEN8xS3RHJGSVpNZjVtUUE?usp=sharing BUG=650456 ==========
Plz fix comments before landing, otherwise this is fine to start. https://codereview.chromium.org/2376163002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/ImageDocument.cpp (right): https://codereview.chromium.org/2376163002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/ImageDocument.cpp:225: m_divElement = HTMLDivElement::create(Node::document()); You are the document, so *this, don't use Node::document https://codereview.chromium.org/2376163002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/ImageDocument.cpp:235: HTMLContentElement* content = HTMLContentElement::create(Node::document()); ditto https://codereview.chromium.org/2376163002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/ImageDocument.cpp:251: m_imageElement->setAttribute(styleAttr, AtomicString(imageStyle.toString())); .toAtomicString(), which is faster if we already have an atomized string with that value.
Comments addressed; waiting on the bots to rebaseline. Thanks! https://codereview.chromium.org/2376163002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/ImageDocument.cpp (right): https://codereview.chromium.org/2376163002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/ImageDocument.cpp:225: m_divElement = HTMLDivElement::create(Node::document()); On 2016/10/13 22:27:23, esprehn wrote: > You are the document, so *this, don't use Node::document Done. https://codereview.chromium.org/2376163002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/ImageDocument.cpp:235: HTMLContentElement* content = HTMLContentElement::create(Node::document()); On 2016/10/13 22:27:23, esprehn wrote: > ditto Done. https://codereview.chromium.org/2376163002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/ImageDocument.cpp:251: m_imageElement->setAttribute(styleAttr, AtomicString(imageStyle.toString())); On 2016/10/13 22:27:23, esprehn wrote: > .toAtomicString(), which is faster if we already have an atomized string with > that value. Ah, was looking for a method like that. Thanks!
The CQ bit was checked by dfalcantara@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by dfalcantara@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by dfalcantara@chromium.org
The CQ bit was checked by dfalcantara@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org, pdr@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2376163002/#ps320001 (title: "Rebasing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Blink] Display images in the center of the screen Code was adapted from MediaDocument.cpp. Additions were made to let the image be zoomed to full width on both mobile and desktop. Had to edit TestExpectations to deal with the virtual tests failing to work with the command line rebaselining tool, but some of the suggested changes are included here. Intent to implement: https://docs.google.com/document/d/1wJPz4kn-9mEFs_ZEM0hAwwr-YzBdW-5hdX1-jkt81... Images of proposed changes: https://drive.google.com/drive/folders/0B8k2SmEN8xS3RHJGSVpNZjVtUUE?usp=sharing BUG=650456 ========== to ========== [Blink] Display images in the center of the screen Code was adapted from MediaDocument.cpp. Additions were made to let the image be zoomed to full width on both mobile and desktop. Had to edit TestExpectations to deal with the virtual tests failing to work with the command line rebaselining tool, but some of the suggested changes are included here. Intent to implement: https://docs.google.com/document/d/1wJPz4kn-9mEFs_ZEM0hAwwr-YzBdW-5hdX1-jkt81... Images of proposed changes: https://drive.google.com/drive/folders/0B8k2SmEN8xS3RHJGSVpNZjVtUUE?usp=sharing BUG=650456 ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== [Blink] Display images in the center of the screen Code was adapted from MediaDocument.cpp. Additions were made to let the image be zoomed to full width on both mobile and desktop. Had to edit TestExpectations to deal with the virtual tests failing to work with the command line rebaselining tool, but some of the suggested changes are included here. Intent to implement: https://docs.google.com/document/d/1wJPz4kn-9mEFs_ZEM0hAwwr-YzBdW-5hdX1-jkt81... Images of proposed changes: https://drive.google.com/drive/folders/0B8k2SmEN8xS3RHJGSVpNZjVtUUE?usp=sharing BUG=650456 ========== to ========== [Blink] Display images in the center of the screen Code was adapted from MediaDocument.cpp. Additions were made to let the image be zoomed to full width on both mobile and desktop. Had to edit TestExpectations to deal with the virtual tests failing to work with the command line rebaselining tool, but some of the suggested changes are included here. Intent to implement: https://docs.google.com/document/d/1wJPz4kn-9mEFs_ZEM0hAwwr-YzBdW-5hdX1-jkt81... Images of proposed changes: https://drive.google.com/drive/folders/0B8k2SmEN8xS3RHJGSVpNZjVtUUE?usp=sharing BUG=650456 Committed: https://crrev.com/8512a83031e6531c976f4af85a28dbeef58bd635 Cr-Commit-Position: refs/heads/master@{#425440} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/8512a83031e6531c976f4af85a28dbeef58bd635 Cr-Commit-Position: refs/heads/master@{#425440} |
