Page MenuHomePhabricator

Parsoid should return the full srcset for high DPI devices
Closed, ResolvedPublic

Description

High DPI devices currently fetch and cache hi-res images on page read, but when switching to edit in VE all we get are the standard res images, which have to be fetched from the server. Parsoid needs to support the srcset attribute to avoid this unnecessary performance hit.

Details

Related Gerrit Patches:
mediawiki/services/parsoid : masterAdd srcset attribute if imageinfo supplies it
mediawiki/extensions/ParsoidBatchAPI : masterProvide URLs for img srcset

Event Timeline

Esanders created this task.Feb 6 2015, 7:45 PM
Esanders raised the priority of this task from to Needs Triage.
Esanders updated the task description. (Show Details)
Esanders added a subscriber: Esanders.
Restricted Application added a project: VisualEditor. · View Herald TranscriptFeb 6 2015, 7:45 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Catrope set Security to None.Feb 6 2015, 7:53 PM
Catrope added a subscriber: ori.
brion added a subscriber: brion.Feb 6 2015, 8:51 PM
brion added a comment.Feb 6 2015, 8:57 PM

Just a warning note -- native browser support for srcset is still a little spotty, so if you're not engaging the jquery.hidpi() transformation some browsers may still show the 1x versions.

Whereas if you do engage the jquery.hidpi() transformation, those non-native browsers will see the 'src' change to one of the values from the 'srcset' at runtime and this may confuse the editor system...

For simplicity I would probably recommend just using srcset, not engaging the jquery.hidpi() polyfill on the edit area, and letting the browsers that don't grok srcset yet (Firefox and IE mostly?) continue to show the low-res images knowing that support is coming.

Firefox - support is present in latest revs but not yet enabled - https://bugzilla.mozilla.org/show_bug.cgi?id=1018389

IE blog note, should appear in IE 12 - http://blogs.msdn.com/b/ie/archive/2014/12/08/status-roadmap-update-srcset-lt-main-gt-element-and-date-inputs-in-development.aspx

(Latest Safari and Chrome should support srcset natively, while some older versions might not.)

brion added a comment.Feb 6 2015, 9:00 PM

Also I kind of prefer things like srcset here to be implementation details that a syntax parser shouldn't know about - sizing the links requires knowing the maximum size of the image, etc.

Ideally it should just ask for a multimedia blob from the wiki's file type handlers and get it back and insert it, otherwise you're going to have to reimplement every new media type perhaps. :(

Our implementation should match the read mode, the main issue here is a performance one with downloading different images for read and edit. If jQuery.hidpi isn't used, or doesn't run in time to prevent the loading of the 1x image then we don't need to worry about it - we just want to catch the case where the browser only ever loaded the 2x image in read mode.

Krinkle added a subscriber: Krinkle.Mar 1 2015, 1:32 AM

Aside from performance, it's also parser parity bug. This is a bug in Parsoid due it not matching MediaWiki PHP parser output. How is this not causing parser tests failures in Parsoid? Or have they been suppressed or excluded somehow?

This is also a blocker for Parsoid to usable for reader views.

cscott added a subscriber: cscott.EditedMar 1 2015, 2:51 AM

This is a dup of T90911, which was forked from T63786.

Like other "Parsoid HTML for views" issues, there is a strong architectural argument to be made that this should *not* be part of the Parsoid DOM, but (like redlinks, default alt attributes, etc) be added as part of a post-processing step on the DOM. ("Parsoid DOM as canonical data representation" -vs- "Parsoid DOM as viewable HTML")

We may, of course, implement that post-processing step inside Parsoid, as some sort of extra option you can pass when you request the DOM for a particular page. Or perhaps all the post-processing should be client-side. Maybe this is a VE bug.

What I'm saying is: there are some architectural issues we need to sort out before we can go off and implement this in Parsoid.

Jdforrester-WMF changed the task status from Open to Stalled.Mar 2 2015, 4:35 AM
Jdforrester-WMF triaged this task as Medium priority.Mar 12 2015, 5:30 PM
bearND added a subscriber: bearND.Oct 16 2015, 3:49 PM
Arlolra moved this task from Needs Triage to In Progress on the Parsoid board.Oct 27 2015, 9:49 PM

Change 249947 had a related patch set uploaded (by Tim Starling):
Add srcset attribute if imageinfo supplies it

https://gerrit.wikimedia.org/r/249947

Change 249948 had a related patch set uploaded (by Tim Starling):
Provide URLs for img srcset

https://gerrit.wikimedia.org/r/249948

Change 249948 merged by jenkins-bot:
Provide URLs for img srcset

https://gerrit.wikimedia.org/r/249948

Change 249947 merged by jenkins-bot:
Add srcset attribute if imageinfo supplies it

https://gerrit.wikimedia.org/r/249947

ssastry closed this task as Resolved.Nov 6 2015, 4:37 AM
ssastry claimed this task.
ssastry added a subscriber: ssastry.

This will go out on Monday.

ssastry reassigned this task from ssastry to tstarling.Nov 6 2015, 4:37 AM
ssastry removed a project: Patch-For-Review.
ssastry removed a subscriber: gerritbot.

This is now in production (went out today since we cancelled Monday's deploy).