Support <picture> in DOM distiller |
||||||||||||||||
Issue descriptionChrome Version: 57.0.2936.0 OS: iOS10. iPhone7 - Enable reader mode in experimental settings - Visit http://www.espn.com/nfl/story/_/id/18206864/redskins-jay-gruden-blasts-team-frustrating-loss-cardinals - Enter reader mode The main image at top of distilled page is empty. There is also another empty image about 1/2 way down, right after the text: "You can't come out flat like that," Baker said.
,
Dec 7 2016
,
Dec 8 2016
The distilled page contains <img src="http://www.espn.com/nfl/story/_/id/18199562/nfl-competition-committee-review-intentional-holding-end-baltimore-ravens-cincinnati-bengals" width="375" height="211"> (which is not an image) and the image vector is empty. This seems a Dom Distiller issue. Assigning to mdjones as wychen is OOO
,
Dec 8 2016
,
Dec 12 2016
,
Dec 13 2016
Hi, did you have time to take a look?
,
Dec 13 2016
I haven't looked into this, but I'll be working full time for at least one week starting 2016-12-21. Does this work for your schedule?
,
Dec 13 2016
Yes, perfect.
,
Dec 15 2016
Passing to wychen as mentioned above.
,
Dec 21 2016
These images are not extracted because they are in <picture> tags. For images on HiDPI devices, we have <img srcset=...> support, but not <picture> currently. This wasn't a big issue before because those who support <picture> usually have a mobile-friendly version already. I'll work on <picture> support for iOS Reader Mode. As for the symptom in #c3, I couldn't reproduce this non-image <img>. The distilled output contains no <img> tags. Could you elaborate how? Thanks!
,
Dec 21 2016
The bug appears when distilling a page on espn with a video as first image. In the link in #3, the video was removed, so you cannot reproduce. But here are 2 example still working: http://www.espn.com/blog/houston-rockets/post/_/id/3333/rockets-loss-doesnt-mean-end-of-the-world Attached distilled version : 18e204320aca2de3a74191e5132e8733.zip and http://www.espn.com/nfl/story/_/page/32for32x161221/2016-nfl-pro-bowl-selections-snubs-all-32-teams attached distilled a893f7ed506b82bbd3c57b8b43875a23.zip
,
Dec 21 2016
Ah. I see what went wrong. The extracted <img> is within <picture>, so it has no src attribute. When converting it to absolute URL, "" becomes the current URL. Thanks for the detailed debugging info!
,
Dec 21 2016
,
Jan 17 2017
Will this be ready for M-57?
,
Jan 18 2017
The CL is ongoing. https://codereview.chromium.org/2638823002/ I'll try to get this done before the branch. Or we could cherry-pick in the first week.
,
Jan 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/527a179fadd4f1604eb003127b89c2d60120213d commit 527a179fadd4f1604eb003127b89c2d60120213d Author: wychen <wychen@chromium.org> Date: Thu Jan 19 02:51:37 2017 Roll DOM Distiller JavaScript distribution package Diff since last roll: https://github.com/chromium/dom-distiller/compare/aab1a1b038...4540f3524d Picked up changes: 4540f35 Support <picture> in image extraction BUG= 671691 Review-Url: https://codereview.chromium.org/2641983004 Cr-Commit-Position: refs/heads/master@{#444609} [modify] https://crrev.com/527a179fadd4f1604eb003127b89c2d60120213d/DEPS [modify] https://crrev.com/527a179fadd4f1604eb003127b89c2d60120213d/third_party/dom_distiller_js/README.chromium
,
Jan 23 2017
,
Jan 23 2017
[Auto-generated comment by a script] We noticed that this issue is targeted for M-57; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-57 label, otherwise remove Merge-TBD label. Thanks.
,
Jan 31 2017
Verified in 58.0.2998.0 canary, iPhone 6+ iOS 10.2, iPad mini 10.1 http://www.espn.com/nfl/story/_/id/18206864/redskins-jay-gruden-blasts-team-frustrating-loss-cardinals First image is replaced with a dot, second image is shown correctly. Looks good in distilled view.
,
Feb 15 2017
|
||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||
Comment 1 by pinkerton@chromium.org
, Dec 6 2016