Bug 180524 – HTML-page with <object type="image/svg+xml" data="foo.svg"> often is blank

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Tobi Reif]

Created attachment 328687 [details]
screenshot_of_three_blanks.png (desc is in the screenshot and in the report)

The page
https://tobireif.com/non_site_stuff/copy_of_snake_pattern_demo_for_bug_report/snake_pattern.html
often is blank in WebKit, eg after zero or more reloads.

Please also see the attached screenshot "screenshot_of_three_blanks.png".

The .svg by itself
http://tobireif.com/non_site_stuff/copy_of_snake_pattern_demo_for_bug_report/snake_pattern.svg
works reliably in WebKit, but
http://tobireif.com/non_site_stuff/copy_of_snake_pattern_demo_for_bug_report/snake_pattern.html
doesn't work in WebKit.

It works reliably in all other browser I tested.

(Please set the correct option for "Component".)

[Tobi Reif]

P.S.

Getting rid of the debouncing code didn't help:

Having just this

["load", "orientationchange", "resize"].forEach(
  function(evt) {
    window.addEventListener(
      evt, makePattern
    );
  }
);

instead of this


["load", "orientationchange"].forEach(
  function(evt) {
    window.addEventListener(
      evt, makePattern
    );
  }
);

window.addEventListener(
  "resize", resizeDebouncer
);

var resizeTimeout;
function resizeDebouncer() {
  if (!resizeTimeout) {
    resizeTimeout = setTimeout(function() {
      resizeTimeout = null;
      makePattern();
    }, 132);
  }
}

didn't help. The page is still blank in WebKit after eg one reload 😐

[Alexey Proskuryakov]

I can reproduce with current shipping Safari. Weird!

[Radar WebKit Bug Importer]

<rdar://problem/35920554>

[Tobi Reif]

Thanks for reproducing/investigating!

I hope the issue can get fixed soon.

[Simon Fraser (smfr)]

This is about the value of innerWidth/innerHeight when the load event fires. The content is blank when they are zero because of the logic in makePattern().

[Simon Fraser (smfr)]

Whether innerHeight/innerWidth are non-zero when the load event fires for the SVG depends on whether the parent document has done layout, which may or may not have happened yet.

Maybe innerHeight/innerWidth on an iframe/object should force layout on the parent frame?

Also, I'm not sure why the resize event is not firing when it does actually get resized.

[Simon Fraser (smfr)]

We don't fire the resize event because that's driven by m_lastViewportSize in FrameView, and we don't even do a layout in the JS that runs from the load event; we do the first layout later, and stash the current view size in m_lastViewportSize on that later layout.

I think the correct fix here is to have innerWidth/innerHeight force layout for frames.

[Simon Fraser (smfr)]

Created attachment 328915 [details]
Patch

[EWS Watchlist]

Comment on attachment 328915 [details]
Patch

Attachment 328915 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5565625

New failing tests:
fast/dom/iframe-inner-size-scaling.html

[EWS Watchlist]

Created attachment 328918 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Simon Fraser (smfr)]

Created attachment 328928 [details]
Patch

[Daniel Bates]

Comment on attachment 328928 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328928&action=review

> LayoutTests/fast/dom/iframe-inner-size-scaling.html:20
> +            scrollbarWidth = UIHelper.isIOS() ? 0 : 15;

Are we not able to compute this? Either directly using the CSSOM or by exposing a window.internals functions? I know scroll bar width computations is not a new problem. We have other tests thats that need to know the scrollbar width. If they hardcode the width then exposing a windows.internals function would be useful.

On another note, I thought we had mock scroll bars. I guess we don’t use the mock scroll bar code on iOS.

> LayoutTests/fast/dom/iframe-innerWidth-expected.txt:8
> +TEST COMPLETE

Please fix this test to not emit this until after all the PASS message as emitting this twice is an indicator that this test can be flaky and depends on testrunner.notifyDone() not being a blocking call that terminates the test.

> LayoutTests/fast/dom/iframe-innerWidth.html:2
> +    <script src="../../resources/js-test-pre.js"></script>

The indentation of the code starting at this line looks weird. I suggest in-indenting it all.

Also, the current trend is to include js-test.js and forgo js-test-pre/js-test-post.js

> LayoutTests/fast/dom/iframe-innerWidth.html:4
> +        description("This tests that innerWidth/innerHeight on an frame window are valid in the load event handler.");

You need to set window.jsTestIsAsync = true. Otherwise, finishJSTest() is called twice and this test is flaky. See above remark in the expected.txt file.

> LayoutTests/fast/dom/iframe-innerWidth.html:6
> +            frame = document.getElementById('iframe');

This is OK as-is. Would be good to pick one quote style and stick with it throughout this test.

[Daniel Bates]

*un-indenting it all

[Daniel Bates]

Comment on attachment 328928 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328928&action=review

> Source/WebCore/ChangeLog:9
> +        The testcase has script that conditionalizes behavior on whether window.innerWidth/Height

Test case  => test case

[Tobi Reif]

Thank you all for working on fixing this!

[Tobi Reif]

The actual URL is https://tobireif.com/demos/snake_pattern/ ,
I hope it will work reliably (on first load and an every reload)
in stable Safari (desktop and mobile) soon.

[Antti Koivisto]

Comment on attachment 328928 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328928&action=review

> Source/WebCore/page/DOMWindow.cpp:1278
> +    if (!m_frame->isMainFrame())
> +        m_frame->document()->updateLayoutIgnorePendingStylesheets();

Frame size doesn't depend on layout of its content (except with frame flattening). There must a better way to fix this than forcing a layout.

[Tobi Reif]

Just in case it's of interest: In the CSS in

https://tobireif.com/non_site_stuff/copy_of_snake_pattern_demo_for_bug_report/snake_pattern.html

and at the actual demo location at

https://tobireif.com/demos/snake_pattern/
in
https://tobireif.com/demos/snake_pattern/snake_pattern.html

there's

object {
  height: 100vh;
  width: 100vw;
}

[Simon Fraser (smfr)]

(In reply to Antti Koivisto from comment #17)
> Comment on attachment 328928 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328928&action=review
> 
> > Source/WebCore/page/DOMWindow.cpp:1278
> > +    if (!m_frame->isMainFrame())
> > +        m_frame->document()->updateLayoutIgnorePendingStylesheets();
> 
> Frame size doesn't depend on layout of its content (except with frame
> flattening). There must a better way to fix this than forcing a layout.

Right but updateLayout() also lays out the parent document, which is necessary to set the frame size.

Laying out the *current* document is unnecessary, agreed. Perhaps I could do something like request offsetWidth of the frame in the parent document. That should hit some optimizations.

[Antti Koivisto]

> Laying out the *current* document is unnecessary, agreed. Perhaps I could do
> something like request offsetWidth of the frame in the parent document. That
> should hit some optimizations.

Yeah, something along those lines should work.

[Antti Koivisto]

Or maybe you simply want to call updateLayoutIgnorePendingStylesheets of the parent document?

[Simon Fraser (smfr)]

Created attachment 329036 [details]
Patch

[Antti Koivisto]

Comment on attachment 329036 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=329036&action=review

> Source/WebCore/page/DOMWindow.cpp:1264
> +    // Force enough layout in the parent document to ensure that the FrameView has been resized.
> +    if (auto* frameElement = this->frameElement())
> +        frameElement->clientHeight();

Since you don't care about the result, you should just call 

frameElement->document().updateLayoutIfDimensionsOutOfDate(*frameElement, HeightDimensionsCheck);

directly instead of relying on it being called as a side effect (and maybe adding a helper for it to Element?).

> Source/WebCore/page/DOMWindow.cpp:1280
> +    if (auto* frameElement = this->frameElement())
> +        frameElement->clientWidth();

Same here with WidthDimensionsCheck

[Simon Fraser (smfr)]

https://trac.webkit.org/r225791

[Tobi Reif]

Thanks all for fixing this!

Is there a way for me to verify that it works for me?
I guess the next Safari Tech Preview?

[Simon Fraser (smfr)]

(In reply to Tobi Reif from comment #25)
> Thanks all for fixing this!
> 
> Is there a way for me to verify that it works for me?
> I guess the next Safari Tech Preview?

Yes (probably 46, not today's).

[Tobi Reif]

Just checked in STP 46 - it works!

Thanks!