Bug 180524 - HTML-page with <object type="image/svg+xml" data="foo.svg"> often is blank
Summary: HTML-page with <object type="image/svg+xml" data="foo.svg"> often is blank
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: Safari 11
Hardware: Unspecified Unspecified
: P2 Blocker
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-07 03:50 PST by Tobi Reif
Modified: 2017-12-21 02:36 PST (History)
11 users (show)

See Also:


Attachments
screenshot_of_three_blanks.png (desc is in the screenshot and in the report) (249.68 KB, image/png)
2017-12-07 03:50 PST, Tobi Reif
no flags Details
Patch (7.46 KB, patch)
2017-12-09 13:36 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.20 MB, application/zip)
2017-12-09 15:01 PST, Build Bot
no flags Details
Patch (7.83 KB, patch)
2017-12-09 17:59 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (8.02 KB, patch)
2017-12-11 14:37 PST, Simon Fraser (smfr)
koivisto: review+
koivisto: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tobi Reif 2017-12-07 03:50:41 PST
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".)
Comment 1 Tobi Reif 2017-12-07 04:29:47 PST
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 😐
Comment 2 Alexey Proskuryakov 2017-12-07 14:09:54 PST
I can reproduce with current shipping Safari. Weird!
Comment 3 Radar WebKit Bug Importer 2017-12-07 15:07:26 PST
<rdar://problem/35920554>
Comment 4 Tobi Reif 2017-12-08 01:24:20 PST
Thanks for reproducing/investigating!

I hope the issue can get fixed soon.
Comment 5 Simon Fraser (smfr) 2017-12-09 11:11:29 PST
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().
Comment 6 Simon Fraser (smfr) 2017-12-09 11:19:56 PST
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.
Comment 7 Simon Fraser (smfr) 2017-12-09 11:42:36 PST
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.
Comment 8 Simon Fraser (smfr) 2017-12-09 13:36:45 PST
Created attachment 328915 [details]
Patch
Comment 9 Build Bot 2017-12-09 15:01:44 PST
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
Comment 10 Build Bot 2017-12-09 15:01:46 PST
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
Comment 11 Simon Fraser (smfr) 2017-12-09 17:59:47 PST
Created attachment 328928 [details]
Patch
Comment 12 Daniel Bates 2017-12-09 18:31:33 PST
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.
Comment 13 Daniel Bates 2017-12-09 18:32:38 PST
*un-indenting it all
Comment 14 Daniel Bates 2017-12-09 18:36:17 PST
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
Comment 15 Tobi Reif 2017-12-10 02:24:16 PST
Thank you all for working on fixing this!
Comment 16 Tobi Reif 2017-12-10 07:04:52 PST
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.
Comment 17 Antti Koivisto 2017-12-10 11:22:29 PST
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.
Comment 18 Tobi Reif 2017-12-11 00:59:27 PST
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;
}
Comment 19 Simon Fraser (smfr) 2017-12-11 08:11:13 PST
(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.
Comment 20 Antti Koivisto 2017-12-11 08:39:53 PST
> 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.
Comment 21 Antti Koivisto 2017-12-11 09:28:59 PST
Or maybe you simply want to call updateLayoutIgnorePendingStylesheets of the parent document?
Comment 22 Simon Fraser (smfr) 2017-12-11 14:37:14 PST
Created attachment 329036 [details]
Patch
Comment 23 Antti Koivisto 2017-12-11 23:19:31 PST
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
Comment 24 Simon Fraser (smfr) 2017-12-12 11:16:30 PST
https://trac.webkit.org/r225791
Comment 25 Tobi Reif 2017-12-13 09:48:28 PST
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?
Comment 26 Simon Fraser (smfr) 2017-12-13 10:00:04 PST
(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).
Comment 27 Tobi Reif 2017-12-21 02:36:31 PST
Just checked in STP 46 - it works!

Thanks!