rendering regression for plots generated by the plotly library in Databricks notebooks
Categories
(Core :: Layout: Scrolling and Overflow, defect)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox-esr68 | --- | unaffected |
| firefox76 | --- | unaffected |
| firefox77 | --- | unaffected |
| firefox78 | + | fixed |
People
(Reporter: jmccrosky, Assigned: emilio)
References
(Regression)
Details
(Keywords: regression)
Attachments
(5 files, 1 obsolete file)
I’ve identified a regression that impacts the rendering of plots generated by the plotly library in Databricks notebooks. This impacts the KPI dashboard used by our leadership (https://go.corp.mozilla.com/kpi-dash). A minimal reproduction is here: https://dbc-caf9527b-e073.cloud.databricks.com/#notebook/285343/ The image at the bottom should appear as a graph going up and to the right but renders as a horizontal line with overlayed and cut-off text.
Mozregression output is:
26:14.78 INFO: Last good revision: b81d3698250c36e63368b833a9363b87c9744334
26:14.78 INFO: First bad revision: 012f25fede5cc41dc810b21e27c3393b382bf9eb
26:14.78 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=b81d3698250c36e63368b833a9363b87c9744334&tochange=012f25fede5cc41dc810b21e27c3393b382bf9eb
The problem has also appeared intermittently in Firefox Release and in Safari, which I can’t explain. It usually works in release though and, as far as I can tell, always fails in recent nightly.
|
||
Comment 1•4 years ago
|
||
[Tracking Requested - why for this release]: Bad regression in some cases.
|
||
Updated•4 years ago
|
|
Assignee | |
Comment 3•4 years ago
|
||
|
|
Assignee | |
Comment 4•4 years ago
|
||
So this is roughly what they're doing:
- They're loading the frame in a zero-size iframe, and then relying on a message from it to get resized.
- The logic to get the size goes something like:
element-size || scrollable-size || body-size. - After my patch the element height is the height of a scrollbar, so we use that instead of the scrollable size and it goes wrong.
|
Reporter | |
Comment 5•4 years ago
|
||
I'll note that we are in contact with DataBricks about this issue and if we think they're doing something wrong, we can communicate that. It would be nice if we can support a temporary fix in that case though, as I suspect they would need some time to deal with this and this breaks some important internal (and probably external) workflows.
If you want to communicate with DataBricks, akomar can help.
|
|
Assignee | |
Comment 6•4 years ago
|
||
Sure, to be clear I don't think they're doing anything wrong (just yet, at least). I'm still reducing the test-case a bit more to see what is causing us to generate a scrollbar in the first place.
|
|
Assignee | |
Comment 7•4 years ago
|
||
(It's just that today I'm afk-ish due to other personal reasons)
|
|
Assignee | |
Comment 8•4 years ago
|
||
I think this does as a reduced test-case. I don't think we should show scrollbars in this case, this looks like a Gecko bug to me.
|
|
Assignee | |
Comment 9•4 years ago
|
||
Ah, so that test-case still fails without my patch. And that sorta makes sense, it's because of line-height... We should investigate that separately probably.
I think this does as a reduced test-case. I don't think we should show scrollbars in this case, this looks like a Gecko bug to me.
In that testcase the svg is display: inline and vertical-align: baseline, so there's space for descenders below it within the block -- thus the scrollbar. If you change it to either display: block or vertical-align: top the scrollbar goes away.
|
|
Assignee | |
Comment 11•4 years ago
|
||
Right. line-height: 0 also "fixes" that test-case, which is what I meant. A bit interesting that Chrome doesn't show scrollbars for that...
This does show the behavior change however. It was a bit tricky to reason about because the page uses tons of percentages and is in quirks-mode so the percentages "jump" across the scroller.
|
|
Assignee | |
Comment 12•4 years ago
|
||
So the SVG causes vertical overflow because it's in a height: 0 scroll frame. That vertical scrollbar causes horizontal overflow and thus an horizontal scrollbar.
Before my patch we used to suppress that scrollbar, but after we don't. I know why now :)
|
|
Assignee | |
Comment 13•4 years ago
|
||
I think one underlying difference here is whether line boxes that have no text cause scrollable overflow. This testcase has two tests -- in the first one the line box has no text in it, and in the second one there is text. Chromium makes only the second one scrollable; Gecko makes both scrollable.
|
|
Assignee | |
Updated•4 years ago
|
|
|
Assignee | |
Comment 15•4 years ago
|
||
Given there's content depending on it.
This brings back the check from before the regression, with a minor
tweak: It checks that the scrollbar fits in the scrollport in both
directions.
Checking it only on one direction, while enough to fix this bug, is not
very consistent (the first test-case I've added to
scrollbar-empty-002.html would fail without it). Chrome matches this
behavior, fwiw.
I had to tweak percent-height-overflowing-image-1.html so that it
doesn't hit scrollbar size limits on GTK (which are a bit ridiculous
IMHO, but oh well).
We could conceivably punt on the tweak / land in another bug if you
want. Your call.
|
||
Comment 17•4 years ago
|
||
Hey all - we actually had this behaviour (from a super speed read of this issue) in NG prior to launch, but then changed it due to web-compat issue. Specifically ruliweb.com was broken.
Change reverted in:
https://chromium-review.googlesource.com/c/chromium/src/+/1774314
ruliweb.com issue:
https://bugs.chromium.org/p/chromium/issues/detail?id=996847
We ignore the bottom half-leading of line-boxes for scrollable overflow.
|
||
Comment 18•4 years ago
|
||
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/83b4973bb9d3 Keep suppressing scrollbars than don't fit in the scrollport for overflow: auto. r=dholbert
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/23647 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
|
||
Updated•4 years ago
|
|
||
Comment 21•4 years ago
|
||
| bugherder | ||
Upstream PR merged by moz-wptsync-bot
Description
•