Closed Bug 1585882 Opened 5 years ago Closed 5 years ago

Regression Bug : SVG graph Display issue (MozRegresion pointed to Bug 1404140)

Categories

(Core :: CSS Parsing and Computation, defect, P2)

71 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox67 --- unaffected
firefox68 --- unaffected
firefox69 --- unaffected
firefox70 + verified
firefox71 --- verified

People

(Reporter: laurent.brichet, Assigned: emilio)

References

(Regression, )

Details

(Keywords: regression)

Attachments

(5 files)

Attached image netatmo.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:71.0) Gecko/20100101 Firefox/71.0

Steps to reproduce:

Go to my.netatmo.com website to manage Thermostats and view historical graph.

Relevant thread on Reddit : https://www.reddit.com/r/firefox/comments/dccexl/svg_graph_display_issue/

MozRegression Report:

2019-10-02T23:16:32: DEBUG : Found commit message:

Bug 1404140 - Remove the GetCSNeedsLayoutFlush flag, as it is unneeded now. r=heycam

MANUAL PUSH: Would need to reorder the stack manually (https://bugzilla.mozilla.org/show_bug.cgi?id=1481539)

Differential Revision: https://phabricator.services.mozilla.com/D40300

2019-10-02T23:16:32: DEBUG : Did not find a branch, checking all integration branches

2019-10-02T23:16:32: INFO : The bisection is done.

2019-10-02T23:16:32: INFO : Stopped

Bisection Information :

app_name: firefoxbuild_date: 2019-08-03 01:14:00.279000build_file: C:\Users\tibus\.mozilla\mozregression\persist\06d909e0de08-pgo--autoland--target.zipbuild_type: inboundbuild_url: https://queue.taskcluster.net/v1/task/UQ_0NynmSSKgxpo8wzewDw/runs/0/artifacts/public%2Fbuild%2Ftarget.zipchangeset: 06d909e0de088e8dd586b4b4be6da83114c93148pushlog_url: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=06d909e0de088e8dd586b4b4be6da83114c93148&tochange=4a49d88894d8d88f87760ac59ae35b2158fab7b2repo_name: autolandrepo_url: https://hg.mozilla.org/integration/autolandtask_id: UQ_0NynmSSKgxpo8wzewDw

Actual results:

The graph layout is corrupted (see screenshot attached).
If I restore the size of the Firefox Windows, the graph is correctly displayed

Expected results:

Graph should be correctly displayer

This is probably some sort of incremental layout issue uncovered by that bug, but I cannot reproduce it since you need an account to log into that site...

Is there any chance you could save the page (with Ctrl + S) and send it to me via email or something like that? Otherwise I cannot really investigate this :(

Flags: needinfo?(laurent.brichet)

(Oh, and thanks a lot for reporting it :))

You are welcome :)

I saved the page, zip and upload it on Firefox Send but I fear you won't see the issue because if I open it I can't see the problem ...
Feel free to ask me any troubleshooting you could need.

https://send.firefox.com/download/c1e93f62971a72a9/#Cxw2ZkNT4iIsYJoHphskwg

Regards,

Laurent

Flags: needinfo?(laurent.brichet)

[Tracking Requested - why for this release]: Regression that would be unfortunate to ship.

I couldn't see it initially, but I can if I manually dismiss the popup using the developer tools, and then change to see weeks instead of days, or vice versa. So... thanks! I have something to debug now :)

Btw, the security of that app is a bit dubious... They're returning the real time data of your device without any kind of authentication (I can see they return data over the network, and I of course am not logged in in any way...). I guess the device id is on the page you saved, I expected a mostly-static page... I'm glad that you used FFSend rather than attaching it to bugzilla.

You can delete it now, and I'll do the same when I'm done debugging this. I'll contact them about this, too... Hopefully I have figured the bug by the time they fix it ;)

Assignee: nobody → emilio
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Flags: needinfo?(emilio)

:-)

I tried to contact them via their forum too with the link to here, we'll see if they react.

Thank you for your time !

Regards,

Laurent

Hmm, they logged me out while debugging, that's not amazing :(

I have an rr recording so I'll try to figure out from there...

Anyhow I think I know why the graph is small, they have width: 100% on the svg, and we're returning that as-is rather than as a pixel value in some circumstances. There's some code doing parseInt(computedStyle.width) to determine the available width, and so you end up with a 100px wide graph rather than an actual wider graph.

Attached file Reduced test-case.

Ok, found it.

What's going on is the following:

  • SVG has display: block.
  • There's a pre-existing <svg> which we remove, but it's inside an inline (the <span> in this case). We realize we're removing from an IB split and determine that we need to go reconstruct the containing block. No other style changes happen.
  • We insert the new <svg>. It doesn't have an insertion point since we've removed it, so we just mark the tree as dirty and carry on (we're going to reconstruct the containing block after all, so we're good).
  • Javascript queries the width. We realize there has been no style change and we don't hit this path (as we're going to reconstruct the ancestor, not just the element itself), so we incorrectly don't flush style, thus we determine we also don't need to do layout, and fail to compute the right pixel width.

I'll work on a fix now, I don't think it should be too hard to fix.

The app can work around it making the <nvd3> elements display: block, for example, which is probably what they intended anyway, and prevents all the block-inside-inline situation.

This test-case shows the pre-existing bug.

This is a pre-existing bug, and this would be enough to fix the website, but
this is still not 100% correct. More on that in a second.

This fixes another edge-case that I thought of while debugging this, I think
this makes our behavior correct now. The comment and test-case should be
self-descriptive.

Comment on attachment 9098687 [details]
Bug 1585882 - Fix needs_frame() check to account for the case where an ancestor of us has been reconstructed regularly, not via lazy frame construction.

Beta/Release Uplift Approval Request

  • User impact if declined: See comment 0.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: Open the test-cases in this bug, verify the output is the expected one.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Just makes an optimization not apply when it's not sound to do so.
  • String changes made/needed: none
Flags: needinfo?(emilio)
Attachment #9098687 - Flags: approval-mozilla-beta?
Attachment #9098688 - Flags: approval-mozilla-beta?
Flags: qe-verify+

Hello,

FYI I just received an answer from Netatmo telling that they are looking on their side to fix it too :-)

Regards,

Laurent

There should be no need to, the fix should make it to Firefox 70.

Plus their code is correct, though I guess the display: block workaround in comment 9 is trivial.

Ok we'll see :)

If your fix is pushed to nightly, i can test it for you if you like.

Can you land this on m-c so we can verify? Then let's think about uplift.

Flags: needinfo?(emilio)

Sure, the patches hadn't been reviewed yet, but have been now.

Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/df80909c8232
Fix needs_frame() check to account for the case where an ancestor of us has been reconstructed regularly, not via lazy frame construction. r=heycam
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/19535 for changes under testing/web-platform/tests

The patch that landed would be enough to fix the issue (we're still discussing the details of the second).

Flags: needinfo?(laurent.brichet)
Keywords: leave-open

Err, sorry, didn't meant to ni? yet, will do when the patch is on Nightly.

Flags: needinfo?(laurent.brichet)
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Hello,

I just tried with the latest nightly available and the issue is fixed !

Thank you very much for this excessively fast resolution :)

Keywords: leave-open

(In reply to Tibuski from comment #26)

Hello,

I just tried with the latest nightly available and the issue is fixed !

Thank you very much for this excessively fast resolution :)

Np, glad to hear that!

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ea10c9b6a81d
Fix the case where where a node with an up-to-date style loses its frame due to a DOM mutation of siblings. r=heycam
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR merged by moz-wptsync-bot
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
QA Whiteboard: [qa-triaged]

I verify this issue using the test case from comment 9, on FF Nightly 71.0a1(2019-10-07) with Windows 10, Mac OS X 10.14 and Ubuntu 18.04 and I can confirm the fix.

Comment on attachment 9098687 [details]
Bug 1585882 - Fix needs_frame() check to account for the case where an ancestor of us has been reconstructed regularly, not via lazy frame construction.

Fix for SVG issue, verified in Nightly, adds tests.
OK for beta 14 uplift.

Attachment #9098687 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9098688 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

I verify this issue using the test case from comment 9, on FF Beta 70.0b14 with Windows 10, Mac OS X 10.14 and Ubuntu 18.04 and I can confirm the fix.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.