Closed Bug 1135812 Opened 9 years ago Closed 9 years ago

picture element does not react to resize/viewport changes

Categories

(Core :: DOM: Core & HTML, defect)

38 Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox37 --- wontfix
firefox38 + wontfix
firefox38.0.5 + wontfix
firefox39 + wontfix
firefox40 + wontfix
firefox41 --- verified
relnote-firefox --- 41+

People

(Reporter: alex, Assigned: jdm)

References

(Blocks 1 open bug)

Details

(Whiteboard: [testday-20150821])

Attachments

(1 file, 6 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:37.0) Gecko/20100101 Firefox/37.0
Build ID: 20150222004034

Steps to reproduce:

Run the following site: http://jsbin.com/fivatoleya/1/edit?html,console,output


Actual results:

The console outputs:

"ok: currentSrc should be data:,b"
"fail: currentSrc should be data:,a, but was instead: data:,b"


Expected results:

The console should output:

"ok: currentSrc should be data:,b"
"ok: currentSrc should be data:,a"
Component: Untriaged → DOM
Product: Firefox → Core
The only really non-trivial part of hooking this up is cleaning up nsDocument's MQ listeners to be usable outside the style system.

ResponsiveImageSelector needs to be able to register MQ listeners with the document and fire some kind of new-selection-available callback at the <img>. We'd also need <source> tags to notify the <img> when their media attr becomes matching, like they do for attr changes currently.

IIRC We also don't fully match the new spec in terms of pending loads and when we fire events, so we should at least make sure we're not firing incorrect events when doing this.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Marcos, your next C++ project? ^_^
Maybe later this year. This is pretty important tho, particularly during development.
Flags: needinfo?(mcaceres)
Marcos, you know anyone that would be willing to take this on? I know we’re down to the wire (and I definitely don’t want to delay this feature), but not re-evaluating on resize led to a lot of developer confusion when Canary worked the same way. I’m worried there’ll be a lot more of that in a stable browser.
Speak of the bugs and the bugs appear: https://github.com/scottjehl/picturefill/issues/447
Andrew, please see above. Perhaps something prioritize for Q2?
Flags: needinfo?(overholt)
jdm told me he's going to try to get this done before 38 goes to beta (30 March) but if not, yeah, it'll have to be in 39.
Flags: needinfo?(overholt)
Setting needinfo on Josh as a reminder.
Flags: needinfo?(josh)
[Tracking Requested - why for this release]: Fairly serious bug in new feature.
Tracking for 38 and 39.  Josh can you explain a bit what the new feature, or spec, is or link me to something about it? Thanks!
Responsive images are an eagerly-awaited new web technology to allow declaring multiple sources for a given image and selecting one of them for the actual request based on media queries. This bug in particular means that the images do not re-select when the media query becomes invalid. http://responsiveimages.org/
Flags: needinfo?(josh)
This is the patch I'm going to throw against try. It makes the testcase in this bug pass.
Assignee: nobody → josh
Status: NEW → ASSIGNED
It passes try but I need to figure out why it doesn't improve the demos in the github issue at all, event with bug 1139554 and bug 1139560 applied.
Comment on attachment 8578202 [details] [diff] [review]
Make picture element react to viewport changes

Review of attachment 8578202 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/ResponsiveImageSelector.cpp
@@ +383,5 @@
> +    }
> +
> +    nsAutoString query(NS_LITERAL_STRING("screen and (device-width: "));
> +    query.AppendInt(computedWidth);
> +    query.AppendLiteral("px");

Should this be "px)"? (Can these be constructed programmatically without constructing a string to parse for every candidate?)

I don't quite understand the intent of this listener though, if I'm reading it right it will only catch when the viewport becomes smaller than our current computed width, which is the result of the | <img sizes=""> | calculation for computed width candidates. We need to calculate and listen for the viewport width threshold that would make the current candidate drop below 1.0x density, warranting a re-selection.

I think we need MQ listeners on:
 - Every mSizeQueries[] member up to and including the selected one, to know when the matching size query changes
 - If our corresponding mSizeValues[] member for the currently selected size query is relative to viewport width (vw unit), *and* we're not selecting the highest density candidate, the calculated width at which our current candidate drops below 1x

An example of an image hitting both conditions would be:
<img sizes="(device-width: 2000px) 1500px, 50vw" srcset="A.jpg 500w, B.jpg 1250w, C.jpg 2000w">

mSizeQueries[0] -> "(device-width: 2000px)"
mSizeQueries[1] -> empty (always matches)
mSizeValues[0] -> 1500px
mSizeValues[1] -> 50vw

Would select (assuming a 1x DPI display):
- A.jpg @ [0-1000px] - Would match sizeQuery [1] and thus compute the image as 0-500px computedWidth.
- B.jpg @ (1000px-2000px) - Still matching sizeQuery[1], but at 1001px the computedWidth of the 50vw image hits 500.5px, at which point we need to switch to B.jpg to maintain >=1x density
- C.jpg @ >=2000px - at 50vw the computedWidth would be 1000, still 1.25x density for B.jpg, but mSizeQueries[0] would now match, and the computed width of the image jumps to 1500px (no longer a vw unit), so the 1250px B.jpg is now only ~0.83x density.

A simpler but slower option than putting MQ listeners on each changeover point would be to re-run SelectImage() on viewport resize, and notify HTMLImageElement if it reaches a different result.
Thanks, that's helpful.
Attachment #8578202 - Attachment is obsolete: true
http://scottjehl.github.io/picturefill/examples/demo-03.html now behaves properly. Of course I now broke the testcase in the original bug description. *sigh*
Now passing the demo pages and the original testcase.
Attachment #8583362 - Attachment is obsolete: true
Comment on attachment 8583978 [details] [diff] [review]
Make picture element react to viewport changes

Review of attachment 8583978 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, some driveby feedback:

::: dom/base/ResponsiveImageSelector.cpp
@@ +295,5 @@
> +      if (mNode->IsHTMLElement(nsGkAtoms::img)) {
> +        static_cast<HTMLImageElement*>(mNode.get())->ViewportChanged();
> +      } else if (mNode->IsHTMLElement(nsGkAtoms::source)) {
> +        static_cast<HTMLSourceElement*>(mNode.get())->ViewportChanged();
> +      } else if (mNode->IsHTMLElement(nsGkAtoms::media)) {

Is this right? IIRC media elements don't use this <source> feature, unless that was introduced recently

::: dom/html/HTMLImageElement.cpp
@@ +1074,5 @@
> +        bool isSourceTag = candidateSource->IsHTMLElement(nsGkAtoms::source);
> +        if (isSourceTag) {
> +          HTMLSourceElement *src = static_cast<HTMLSourceElement*>(candidateSource);
> +          candidateIsUsable = src->MatchesCurrentMedia();
> +        }

This skips the type check in TryCreateResponsiveSelector(). the bit of that function that decides whether to proceed with creating a selector should be split out to use it here.
This patch is large because of a lot of cycle-collector-related boilerplate. It's quite simple conceptually - when there is a responsive image, it creates a media query that matches any viewport size except the current one. When the media query matches, it notifies the responsive image to perform resource selection again, at which point a new media query is created and the process begins again.
Attachment #8583978 - Attachment is obsolete: true
Comment on attachment 8584737 [details] [diff] [review]
Make picture element react to viewport changes

David, can you review the changes to the media query list code?
Attachment #8584737 - Flags: review?(dbaron)
Comment on attachment 8584737 [details] [diff] [review]
Make picture element react to viewport changes

Johnny, are you comfortable reviewing the DOM changes? I haven't heard back from johns yet about whether he wants to do this review officially.
Attachment #8584737 - Flags: review?(jst)
This feels like a lot more code than I'd have expected.  Instead of the approach in comment 22, why not just keepahash table or linked list of the elements you might need to reevaluate, and go through that list in a function called from nsPresContext::SetVisibleArea?
Flags: needinfo?(josh)
Josh are you still working on this? If we want to take it for 38 it will need some testing and should land soon (maybe before beta 5).
I know, and I haven't been able to rewrite it according to David's suggestions yet. The responsive image stuff is generally at risk; I'm going to make a call about whether to deactivate it on release soon.
More like this?
Attachment #8592083 - Flags: review?(dbaron)
Attachment #8584737 - Attachment is obsolete: true
Attachment #8584737 - Flags: review?(jst)
Attachment #8584737 - Flags: review?(dbaron)
Flags: needinfo?(josh)
Josh and I spoke about this and it seems unlikely we're going to crash land changes here on beta at this late date.
Comment on attachment 8592083 [details] [diff] [review]
Make picture element react to viewport changes

nsDocument.cpp:

>+  if (!mResponsiveContent.PutEntry(aContent)) {
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }

This PutEntry variant (without mozilla::fallible) never returns null.

Also, it seems like maybe you should assert that
aContent->IsHTMLElement(nsGkAtoms::img), since calling it with anything
else won't do anything.


nsDocument.h:

>+  nsresult AddResponsiveContent(nsIContent* aContent) override;
>+  // Removes an element from mResponsiveContent when the element is
>+  // removed from the tree.
>+  void RemoveResponsiveContent(nsIContent* aContent) override;
>+  void NotifyViewportChanged() override;

Current convention is to write "virtual" on these, although we're
talking about changing that and mass-replacing the tree.

>+  // A hashtable of responsive images keyed by address pointer.

Maybe worth using the term "set" somewhere in this comment, since
it's being used as a set rather than a key->value map (i.e., there's
no value).

>   if (aNullParent && GetParent() &&
>       GetParent()->IsHTMLElement(nsGkAtoms::picture) &&
>       HTMLPictureElement::IsPictureEnabled()) {
>+    nsIDocument* doc = GetOurOwnerDoc();
>+    if (doc) {
>+      doc->RemoveResponsiveContent(this);
>+    }
>     // Being removed from picture re-triggers selection, even if we
>     // weren't using a <source> peer
>     QueueImageLoadTask();
>   }

You don't want the |aNullParent| condition here for your check, but
you do want it for the existing check, so it should probably move
into a second if around the QueueImageLoadTask.

(With this wrong, you'll leave any img elements whose parent picture
is removed from the tree in the map, and leak them.)

>+        bool isSourceTag = candidateSource->IsHTMLElement(nsGkAtoms::source);
>+        if (isSourceTag) {

The variable here doesn't seem to add much; maybe just put
the condition in the if?

(And maybe also use && instead of two nested ifs.)

(Perhaps somebody more familiar with picture should also review
this bit?)




Finally, given that this is really about any media query changes and
not just viewport changes (as I understand it), the ViewportChanged
(etc.) methods should be called MediaFeatureValuesChanged.

Also, nsDocument.h should have a comment that says that its
NotifyMediaFeatureValuesChanged method notifies the responsive content
added by AddResponsiveContent (rather than being a general notification
mechanism for changes in media feature values).   It might even be good
to use a more specific name if you can come up with one.  (This is
important since some notifications go through the document to get to the
pres context; in this case it's going through the pres context to get
to the document.)


r=dbaron with that, and sorry for the delay getting to it
Attachment #8592083 - Flags: review?(dbaron) → review+
Too late for 38 but if it is safe, I could take a patch for 38.0.5.
Don't forget to land this. :-)
Flags: needinfo?(josh)
Addressed all comments except for the more specific name; I haven't been able to come up with anything better.
Attachment #8592083 - Attachment is obsolete: true
Flags: needinfo?(josh)
Final version + test.
Attachment #8602730 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/ef3e09a6a0c3
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Totally understand if you can’t say, but what’s the likelihood of this landing in 38.0.5? We’re doing a little planning on the Picturefill side; we might opt 38.0.0 into the polyfilling behavior to work around the issue, but not if the fix is coming along any moment now.
This looks like a fairly large change to take in 38.0.5. As Sylvestre noted in comment 32, we can consider a fix but only if it's safe. I'm not sure this meets that description. Unless there is a really strong case for taking this in 38.0.5, I suggest that we instead target Firefox 39, which is scheduled to release on June 30. Targeting 39 should give us most of the Beta cycle to identify fallout.
We got a slew of issues in the Picturefill repo when `picture` landed without reevaluating on viewport resize in Canary; I’m just worried about developer expectations when we run into the same situation in a stable release.

Having this land in 39 seems reasonable. The fix is pretty simple from our side (https://github.com/scottjehl/picturefill/blob/afarkas-pf3-proposal/src/plugins/gecko-picture/pf.gecko-picture.js), but we’ve been hoping to avoid adding too much UA-specific biz—sounds like it would be short-lived in any case.
Josh, are there plans to land this on aurora and/or beta?
Flags: needinfo?(josh)
Yes, I'd like to get this into 39 and 40.
Flags: needinfo?(josh)
Depends on: 1163911
This doesn't apply to bare <img> elements using just srcset (As example is http://www.webkit.org/demos/srcset/), they only pick up the viewport changes on reload. The check in the landed patch only checks if the image has a parent <picture> element, not if it actually needs to respond to viewport changes.

<img srcset=...>

Doesn't work, but

<picture><img srcset=...></picture>

Does, the patch doesn't depend on the presence of any <source> elements that would require the <picture> element.
Blocks: 1166138
(In reply to Alex from comment #45)
> This doesn't apply to bare <img> elements using just srcset (As example is
> http://www.webkit.org/demos/srcset/), they only pick up the viewport changes
> on reload. The check in the landed patch only checks if the image has a
> parent <picture> element, not if it actually needs to respond to viewport
> changes.
> 
> <img srcset=...>
> 
> Doesn't work, but
> 
> <picture><img srcset=...></picture>
> 
> Does, the patch doesn't depend on the presence of any <source> elements that
> would require the <picture> element.

I filed bug 1166138 for this
Marking 40 as affected. 
Josh can you fill out a request for approval for uplift if this is ready to go?
Flags: needinfo?(josh)
I'm not yet confident about it, since I haven't figured out the cause of the very frequent failures in bug 1163911 which was introduced by this patch.
Flags: needinfo?(josh)
@Josh: Not really sure what the cause is either, but to me it looks like a bug in MediaQueryList (not the respimg implementation). 

I tried to reproduce your issue and if this issue appears, the JS API `matchesMedia` result is also wrong.

In case "data:,a" is expected, the media query (min-width: 150px) should match, but it doesn't. 
From log I get this: "(min-width: 150px)", matches: false

I'm using this test: https://gist.github.com/aFarkas/ff13fe7f0df81dbfc021

And in case this error happens I get the following logs:

``
data:,c is fine gecko.html:18:5
innerWidth: 50 MediaQueryList { media: "(min-width: 150px)", matches: false } MediaQueryList { media: "(min-width: 100px)", matches: true } MediaQueryList { media: "(min-width: 10px)", matches: true } gecko.html:22:1
-------------- gecko.html:23:4
it was data:,c, but we expected: data:,a gecko.html:20:5

innerWidth: 200 MediaQueryList { media: "(min-width: 150px)", matches: false } MediaQueryList { media: "(min-width: 100px)", matches: true } MediaQueryList { media: "(min-width: 10px)", matches: true } gecko.html:22:1
-------------- gecko.html:23:4
it was data:,a, but we expected: data:,b gecko.html:20:5

innerWidth: 120 MediaQueryList { media: "(min-width: 150px)", matches: false } MediaQueryList { media: "(min-width: 100px)", matches: true } MediaQueryList { media: "(min-width: 10px)", matches: true } gecko.html:22:1
-------------- gecko.html:23:4
data:,b is fine gecko.html:18:5
innerWidth: 120 MediaQueryList { media: "(min-width: 150px)", matches: false } MediaQueryList { media: "(min-width: 100px)", matches: true } MediaQueryList { media: "(min-width: 10px)", matches: true } gecko.html:22:1
--------------
``

This is the expected result:

``
data:,c is fine gecko.html:18:5
innerWidth: 50 MediaQueryList { media: "(min-width: 150px)", matches: false } MediaQueryList { media: "(min-width: 100px)", matches: false } MediaQueryList { media: "(min-width: 10px)", matches: true } gecko.html:22:1
-------------- gecko.html:23:4
data:,a is fine gecko.html:18:5
innerWidth: 200 MediaQueryList { media: "(min-width: 150px)", matches: true } MediaQueryList { media: "(min-width: 100px)", matches: true } MediaQueryList { media: "(min-width: 10px)", matches: true } gecko.html:22:1
-------------- gecko.html:23:4
data:,b is fine gecko.html:18:5
innerWidth: 120 MediaQueryList { media: "(min-width: 150px)", matches: false } MediaQueryList { media: "(min-width: 100px)", matches: true } MediaQueryList { media: "(min-width: 10px)", matches: true } gecko.html:22:1
--------------
``

Additionally the bug seems to happen mostly, if you open the page from another one in a hidden tab/background tab.
I didn't look closely at what you described -- but it's remotely possible that the known bugs described in bug 1089417 related to asynchronous handling of dynamic media query changes could be related.  (If the changes for picture are handled synchronously, then it wouldn't be related.)  There's an idea in the bug about how to fix them, but I haven't had a chance to try it yet.
Josh are you still aiming to fix this in 39 or is 40 more realistic? We are heading into beta 4 (of 7) so this may end up as a wontfix for 39.
Flags: needinfo?(josh)
Josh and I just spoke and 40 seems more likely/safe at this point.
Flags: needinfo?(josh)
Andrew, it is possible to have the uplift request to 40(aurora)? thanks!
Flags: needinfo?(overholt)
Hmm, I thought there was more work to be done here. Let me get back to you, Sylvestre.
Josh is away right now and given the work remaining we're going to have to wait on uplift to 40.  If the fixes are simple when he returns, we can consider uplifting to 40 but by then it'll be beta.  Sorry, not a great situation.
Flags: needinfo?(overholt)
Andrew, do you know when Josh is back?
We have an interesting bug report over at picturefill. It says that on Firefox Aurora for Android the following `picture` is loaded incorrectly even initially:

```
<picture>
	<source srcset="https://placeholdit.imgix.net/~text?txtsize=33&txt=landscape&w=350&h=150&txttrack=0" media="(orientation: landscape)" />
	<source srcset="https://placeholdit.imgix.net/~text?txtsize=33&txt=portrait&w=350&h=150&txttrack=0" media="(orientation: portrait)" />
	<img />
</picture>
```

Here is the link to the pf issue:
https://github.com/scottjehl/picturefill/issues/524

and here a link to the page, where you should be able reproduce this:
http://ny.olsvik-kirke.no/

I had not time to look into this. So I didn't created an issue for you, but maybe this might interest you.
cf comment #57
Flags: needinfo?(overholt)
(In reply to Sylvestre Ledru [:sylvestre] from comment #57)
> Andrew, do you know when Josh is back?

Next week
Flags: needinfo?(overholt)
So far, we’ve had the following related issues mistakenly reported as issues with the Picturefill polyfill:

https://github.com/scottjehl/picturefill/issues/501
https://github.com/scottjehl/picturefill/issues/503
https://github.com/scottjehl/picturefill/issues/504
https://github.com/scottjehl/picturefill/issues/509
https://github.com/scottjehl/picturefill/issues/512
https://github.com/scottjehl/picturefill/issues/513
https://github.com/scottjehl/picturefill/issues/517

Along with a handful of confused tweets. Reloading the browser in order to get layout-appropriate images is not expected behavior when testing—developers are just seeing responsive images as “not working” in current versions of Firefox. 

Despite their newness, responsive images are a very high-visibility feature. I can understand someone needing to have ownership of this issue, but holding off on addressing it while that person is unavailable is causing significant developer confusion.
Mat, I'm not sure what you're asking to be addressed.  This bug as reported is fixed, and the fix will be shipped in Firefox 41, modulo whatever the issue described in comment 58 is, which could really use a separate bug with clear steps to reproduce.  The thing that's blocked on Josh getting back is maybe backporting from our development tree to a stabilization branch so that we can ship the fix earlier than 41, and the main reason that's blocked on Josh is that it requires a clear evaluation of the stability risks.  Note that he's getting back in 3 days, by the way, so it's not like asking someone else to get up to speed on this stuff and do the evaluation would particularly help.
Alexander, do you mind filing a separate bug on comment 58 with a clear description of what the problem is?  It sounds from the picturefill issue like it's specific to Firefox on Android, but it would be good to understand clearly which version of Firefox on Android is being tested and what the steps to reproduce the problem are.  I can try to file that bug as needed if you can't do it for some reason (just let me know), but I won't have access to Firefox on Android for a few days so won't be able to file it until then.  If you do file a new bug, please comment here with the bug number?
Flags: needinfo?(info)
I absolutely appreciate that this bug was addressed some time ago, Boris. Looking forward to seeing it land.
(In reply to Boris Zbarsky [:bz] from comment #63)
> If you do file a new bug, please comment here with the bug number?

https://bugzilla.mozilla.org/show_bug.cgi?id=1178106
Flags: needinfo?(info)
> The thing that's blocked on Josh getting back is maybe backporting from our development tree to a
> stabilization branch so that we can ship the fix earlier than 41, and the main reason that's blocked on
> Josh is that it requires a clear evaluation of the stability risks.  Note that he's getting back in 3
> days, by the way, so it's not like asking someone else to get up to speed on this stuff and do the
> evaluation would particularly help.

Is backporting to a stable branch still pending?
Flags: needinfo?(josh)
Flags: needinfo?(bzbarsky)
We can also file a bug for backporting this if you want.
Going to let jdm make the call on whether this is OK to land on beta.
Flags: needinfo?(bzbarsky)
Comment on attachment 8604348 [details] [diff] [review]
Make picture element react to viewport changes

Approval Request Comment
[Feature/regressing bug #]: 1017875
[User impact if declined]: Frequent web developer confusion over an incomplete implementation of a long-requested feature.
[Describe test coverage new/current, TreeHerder]: WPT tests, new tests, modified existing tests.
[Risks and why]: Memory leaks hitherto undiscovered by automated tests and nightly/aurora testers.
[String/UUID change made/needed]: None

This should be approved or not approved in conjunction with bug 1163911.
Flags: needinfo?(josh)
Attachment #8604348 - Flags: approval-mozilla-beta?
Comment on attachment 8604348 [details] [diff] [review]
Make picture element react to viewport changes

After speaking with jdm, given that we're talking about taking the fix in beta6, I think it's safest at this point to ship 40 with this issue and ship the fix in 41. Beta-
Attachment #8604348 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Release Note Request (optional, but appreciated)
[Why is this notable]: Known issue that impacts devs.
[Suggested wording]: Fixed: picture element does not react to resize/viewport changes
[Links (documentation, blog post, etc)]:
QA Whiteboard: [good first verify]
I have reproduced this bug with Firefox Nightly 38.0a1 (Build ID: 20150223030231) on 
windows 8.1 pro 64-bit with the instructions from comment 0 .

Verified as fixed with Latest Firefox Nightly 43.0a1(Build ID: 20150822030206)

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0

Verified as fixed with latest Firefox beta 41.0b3 (Build ID: 20150820142145)

Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:41.0) Gecko/20100101 Firefox/41.0
Whiteboard: [testday-20150821]
Successfully reproduce the bug on : Firefox 39.0; Build ID 20150629114836; User Agent 	Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:39.0) Gecko/20100101 Firefox/39.0

The fix works me on Firefox ‌41.0.1 ; Build ID 20150929144111; User Agent Mozilla/5.0 (X11; Linux i686; rv:41.0) Gecko/20100101 Firefox/41.0

Based on comment 74 , I am marking the fixed as verified.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: