When the page's content is zoomed in, only the content of frames is scaled, not the frames themselves. The url I linked to is a bug on chromium, but this happens on all webkit based browsers I've tried it on. Pretty much, content starts getting cut off when zooming in on websites that use frames because frames aren't themselves being scaled, only their content.
@reporter: could you provide a reduced test case?
Created attachment 111212 [details] adding test case
Created attachment 111213 [details] Patch
Comment on attachment 111213 [details] Patch Why there is no tests to this?
Please make the test case you attached above a layout test but avoid using text (you may have platform dependent test results).
Comment on attachment 111213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111213&action=review review- because this patch does not have a regression test > Source/WebCore/ChangeLog:8 > + No new tests. Why? Bug fixes need to have tests.
Created attachment 111398 [details] Patch
Is the zoom ratio of 1.2 when zooming in/out platform specific?
Comment on attachment 111398 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111398&action=review > Source/WebCore/ChangeLog:7 > + Please explain why you're making this change. r- because of the lack of explanation. > Source/WebCore/rendering/RenderFrameSet.cpp:218 > - gridLayout[i] = max(grid[i].value(), 0); > + gridLayout[i] = max(static_cast<int>(grid[i].value() * style()->effectiveZoom()), 0); Are you sure we just want to cast it to int? Don't we need to use on of those fancy snapping functions listed on http://trac.webkit.org/wiki/LayoutUnit? > LayoutTests/ChangeLog:10 > + * fast/frames/frame-set-zoom-page-expected.png: Added. > + * fast/frames/frame-set-zoom-page-expected.txt: Added. > + * fast/frames/frame-set-zoom-page.html: Added. There is no way to test this using a ref test? > LayoutTests/fast/frames/frame-set-zoom-page.html:1 > +<html> Missing DOCTYPE.
+eae, +leviw because this change is zoom related.
Comment on attachment 111398 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111398&action=review >> Source/WebCore/rendering/RenderFrameSet.cpp:218 >> + gridLayout[i] = max(static_cast<int>(grid[i].value() * style()->effectiveZoom()), 0); > > Are you sure we just want to cast it to int? Don't we need to use on of those fancy snapping functions listed on http://trac.webkit.org/wiki/LayoutUnit? I'm not entirely sure what you are trying to do here but it seems like you'd want to use snapSizeToPixel or somehow account for the rounding error or the last column/row will end up with all the extra space from the accumulated flooring.
Created attachment 138498 [details] Updated patch Thanks for reviewing.
Comment on attachment 138498 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=138498&action=review > Source/WebCore/ChangeLog:9 > + When frame size in frameset is specified with fixed length, we need to scale > + frame size too. Currently on page zoom, only content gets zoomed. And why do we want to zoom frame? Is there any spec that mandates this behavior or is there some compat. issue? I understand what change you're making but it's not clear to me why we want to make this change.
(In reply to comment #13) > (From update of attachment 138498 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=138498&action=review > > > Source/WebCore/ChangeLog:9 > > + When frame size in frameset is specified with fixed length, we need to scale > > + frame size too. Currently on page zoom, only content gets zoomed. > > And why do we want to zoom frame? Is there any spec that mandates this behavior or is there some compat. issue? > I understand what change you're making but it's not clear to me why we want to make this change. Firefox, IE9 and Opera zoom frame. This behaviour should be consistent across browsers. Please correct me if I am wrong.
Comment on attachment 138498 [details] Updated patch So you are handling frame (within frameset) only. Is iframe's already covered, or is it a different history?
(In reply to comment #15) > (From update of attachment 138498 [details]) > So you are handling frame (within frameset) only. Is iframe's already covered, or is it a different history? Yes. iframes are already covered.
Created attachment 141859 [details] Rebased Patch
Comment on attachment 141859 [details] Rebased Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141859&action=review > Source/WebCore/rendering/RenderFrameSet.cpp:218 > + int fixedLen = snapSizeToPixel(grid[i].value() * style()->effectiveZoom(), totalFixed); Emil, you suggested to use snapSizeToPixel but it seems misguided as snapSizeToPixel is used for converting sizes not 1-dimensional coordinates AFAICT. It won't give any guarantees on whether we account for all the available space or spread the error for that matter. This is unfortunately a common issue (tables are also impacted). More to the point, snapSizeToPixel forces us to convert the values to FractionalUnits and then back to int which seems wrong and wasteful. I would just go with lroundf or ceilf in this case. > LayoutTests/ChangeLog:11 > + * fast/frames/frame-set-zoom-page-expected.html: Added. > + * fast/frames/frame-set-zoom-page.html: Added. This covers only rows. Could we have a similar one for cols as they are also impacted by this change? > LayoutTests/fast/frames/frame-set-zoom-page.html:4 > + <title> FrameSet Zoom Test</title> This title doesn't add much, it should be dropped. > LayoutTests/fast/frames/frame-set-zoom-page.html:14 > +</head> Ideally we need in the output: * Bug id, bonus points if it's clickable. * Bug title. * What are your expectations for the test to pass or fail? (here it looks like: the frameset size should increase to account for the zoom) * Manual testing / warning when not run under DRT (if applicable) If it cannot be in the output (which I feared because of the <frameset>), it should be in the markup. > LayoutTests/fast/frames/frame-set-zoom-page.html:15 > + <frameset rows="50,*" style="background-color: red"> I don't understand the need for the red background. Currently we don't account for the zoom factor so the background will not be seen. In my simple mind, 'red' means failure and here it doesn't seem to equate that.
Created attachment 142632 [details] Updated Patch Fixed review comments and rebased.
Comment on attachment 142632 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142632&action=review I would like to see another round. > Source/WebCore/rendering/RenderFrameSet.cpp:218 > + gridLayout[i] = max(static_cast<int>(lroundf(grid[i].value() * style()->effectiveZoom())), 0); no static_cast please. If it's needed to compile, it's better to specify which specialization of max you are using: gridLayout[i] = max<int>(lroundf(grid[i].value() * style()->effectiveZoom()), 0); > LayoutTests/fast/frames/frame-set-zoom-page.html:5 > + function init() { It's not really an initialization (let's not abbreviate), more than the test itself. Better name: zoomIn, testZooming, ... > LayoutTests/fast/frames/frame-set-zoom-page.html:17 > + <!-- > + Test case for https://bugs.webkit.org/show_bug.cgi?id=64883. Zoom only scales frames' content, not the frames themselves. > + This test passes if there is horizontal frame of 60px height at the top and below it a vertical frame of width 60px to left. > + --> This should be dumped into the ouput as it's useful for anyone looking at the test. The expected file will need to be updated accordingly. Please add somewhere how to test the page manually.
Created attachment 144031 [details] Updated Patch Fixed review comments. I have changed the test case to zoom out so that it can be easily tested manually.
Created attachment 144035 [details] Updated Patch changed to use ahem font.
Comment on attachment 144035 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144035&action=review > LayoutTests/fast/frames/frame-set-zoom-page-expected.html:3 > +<!-- Reference test case for https://bugs.webkit.org/show_bug.cgi?id=64883. Zoom only scales frame's content, not the frames themselves. --> This is unneeded as we dump the info now. > LayoutTests/fast/frames/frame-set-zoom-page-expected.html:8 > + <frame src="data:text/html,<body bgcolor='green' style='margin: 0px;'><div style='font: 15px ahem;'>Test for <a href='https://bugs.webkit.org/show_bug.cgi?id=64883'>BUG 64883</a>: Zoom only scales frame's content, not the frames themselves. There should not be any red on this page when zoomed out.</div></body>"> Could you explain the rationale for using Ahem here? I wouldn't expect the font family / size to matter on a ref-test and that makes your output unreadable to the naked eye.
Created attachment 144263 [details] Updated Patch
Comment on attachment 144035 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144035&action=review >> LayoutTests/fast/frames/frame-set-zoom-page-expected.html:3 >> +<!-- Reference test case for https://bugs.webkit.org/show_bug.cgi?id=64883. Zoom only scales frame's content, not the frames themselves. --> > > This is unneeded as we dump the info now. Done. >> LayoutTests/fast/frames/frame-set-zoom-page-expected.html:8 >> + <frame src="data:text/html,<body bgcolor='green' style='margin: 0px;'><div style='font: 15px ahem;'>Test for <a href='https://bugs.webkit.org/show_bug.cgi?id=64883'>BUG 64883</a>: Zoom only scales frame's content, not the frames themselves. There should not be any red on this page when zoomed out.</div></body>"> > > Could you explain the rationale for using Ahem here? I wouldn't expect the font family / size to matter on a ref-test and that makes your output unreadable to the naked eye. Changed to monospace. Font size should matter because text also gets zoomed.
Comment on attachment 144263 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144263&action=review > Source/WebCore/ChangeLog:10 > + > + When frame size in frameset is specified with fixed length, we need to scale > + frame size too. Currently on page zoom, only content gets zoomed. > + Can you please test this with frameflattening as well?
(In reply to comment #26) > (From update of attachment 144263 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144263&action=review > > > Source/WebCore/ChangeLog:10 > > + > > + When frame size in frameset is specified with fixed length, we need to scale > > + frame size too. Currently on page zoom, only content gets zoomed. > > + > > Can you please test this with frameflattening as well? kenneth, can I test frameflattening manually? I tried DRT testing using fast/frames/flattening/frameset-flattening-simple.html. If I call eventSender.zoomPageOut() with setFrameFlatteningEnabled(true), frame width should still be 800px?
I am able to reproduce this bug in Safari 16.4, hence changing this to "New" and also it was fixed in Blink with this commit: https://chromium.googlesource.com/chromium/src.git/+/9c7ef525d830b014626d787f3060512c2d3251a2
PR Attempt - https://github.com/WebKit/WebKit/pull/13637 Based on Uday's patch.
(In reply to Ahmad Saleem from comment #29) > PR Attempt - https://github.com/WebKit/WebKit/pull/13637 > > Based on Uday's patch. Based on discussion with Simon, the following PR would lead to cases where zoomed-in frame would be unscrollable in certain windows size and might be more dominant cases like mobile devices and since zoom settings are preserved throughout sessions. It would need user education to educate them on how to reset the zoom on such websites. Currently, all browsers (as far as I tested) leads to this issue where at certain window sizes, the frames on screen might be unscrollable after zoomed-in and leading the websites being unaccessible. At this time, we need to be conservation in approach and don't want to break the websites with situation, which is not easy to reset. Hence, I am closing my PR and documenting the issue with current approach. Additionally, 'frameset' are not as popular as other technologies: https://chromestatus.com/metrics/feature/timeline/popularity/3009 So the impact of this world be not significant. Although, this is one example of broken site, where zooming-in leads to 'buttons' (Menu) not being accessible in Safari: https://fsi.gov.in/LATEST-WB-SITE/fsi-main-pg-frm.htm
<rdar://problem/122588360>