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".)
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 😐
I can reproduce with current shipping Safari. Weird!
<rdar://problem/35920554>
Thanks for reproducing/investigating! I hope the issue can get fixed soon.
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().
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.
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.
Created attachment 328915 [details] Patch
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
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
Created attachment 328928 [details] Patch
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.
*un-indenting it all
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
Thank you all for working on fixing this!
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 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.
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; }
(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.
> 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.
Or maybe you simply want to call updateLayoutIgnorePendingStylesheets of the parent document?
Created attachment 329036 [details] Patch
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
https://trac.webkit.org/r225791
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?
(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).
Just checked in STP 46 - it works! Thanks!