Bug 64883

Summary:

Command-+ zoom on a frameset page fails to resize the frames, only resizes their content

Product:

WebKit

Reporter:

acquire16

Component:

Frames

Assignee:

Nobody <webkit-unassigned>

Status:

NEW

Severity:

Normal

CC:

ahmad.saleem792, ap, bfulgham, eae, eric, fsamuel, jchaffraix, koivisto, leviw, rniwa, simon.fraser, tonikitoo, udaykiran4u, webkit-bug-importer

Priority:

Keywords:

BrowserCompat, InRadar

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

URL:

http://code.google.com/p/chromium/issues/detail?id=82967&q=frame%20zoom&colspec=ID%20Stars%20Pri%20Area%20Feature%20Type%20Status%20Summary%20Modified%20Owner%20Mstone%20OS

Bug Depends on:

Bug Blocks:

68075

Attachments:

Description	Flags
adding test case	none
Patch	none
Patch	none
Updated patch	none
Rebased Patch	none
Updated Patch	none
Updated Patch	none
Updated Patch	none
Updated Patch	none

acquire16

Reported 2011-07-20 12:13:06 PDT

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.

Attachments
adding test case (178 bytes, text/html) 2011-10-16 22:33 PDT, Uday Kiran	no flags	Details
Patch (1.50 KB, patch) 2011-10-16 22:35 PDT, Uday Kiran	no flags	Details Formatted Diff Diff
Patch (4.75 KB, patch) 2011-10-18 00:11 PDT, Uday Kiran	no flags	Details Formatted Diff Diff
Updated patch (3.92 KB, patch) 2012-04-23 23:26 PDT, Uday Kiran	no flags	Details Formatted Diff Diff
Rebased Patch (3.92 KB, patch) 2012-05-14 23:11 PDT, Uday Kiran	no flags	Details Formatted Diff Diff
Updated Patch (4.36 KB, patch) 2012-05-17 22:36 PDT, Uday Kiran	no flags	Details Formatted Diff Diff
Updated Patch (5.03 KB, patch) 2012-05-25 03:33 PDT, Uday Kiran	no flags	Details Formatted Diff Diff
Updated Patch (5.02 KB, patch) 2012-05-25 03:51 PDT, Uday Kiran	no flags	Details Formatted Diff Diff
Updated Patch (4.70 KB, patch) 2012-05-27 21:55 PDT, Uday Kiran	no flags	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Antonio Gomes

Comment 1 2011-10-06 07:08:49 PDT

@reporter: could you provide a reduced test case?

Uday Kiran

Comment 2 2011-10-16 22:33:30 PDT

Created attachment 111212 [details] adding test case

Uday Kiran

Comment 3 2011-10-16 22:35:25 PDT

Created attachment 111213 [details] Patch

Antonio Gomes

Comment 4 2011-10-17 07:14:41 PDT

Comment on attachment 111213 [details] Patch Why there is no tests to this?

Fady Samuel

Comment 5 2011-10-17 07:17:03 PDT

Please make the test case you attached above a layout test but avoid using text (you may have platform dependent test results).

Darin Adler

Comment 6 2011-10-17 12:10:17 PDT

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.

Uday Kiran

Comment 7 2011-10-18 00:11:42 PDT

Created attachment 111398 [details] Patch

Uday Kiran

Comment 8 2011-10-18 00:21:09 PDT

Is the zoom ratio of 1.2 when zooming in/out platform specific?

Ryosuke Niwa

Comment 9 2012-04-22 22:30:46 PDT

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.

Ryosuke Niwa

Comment 10 2012-04-22 22:31:11 PDT

+eae, +leviw because this change is zoom related.

Emil A Eklund

Comment 11 2012-04-23 10:25:01 PDT

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.

Uday Kiran

Comment 12 2012-04-23 23:26:09 PDT

Created attachment 138498 [details] Updated patch Thanks for reviewing.

Ryosuke Niwa

Comment 13 2012-04-23 23:37:19 PDT

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.

Uday Kiran

Comment 14 2012-04-23 23:52:07 PDT

(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.

Antonio Gomes

Comment 15 2012-04-24 06:54:59 PDT

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?

Uday Kiran

Comment 16 2012-04-24 21:54:09 PDT

(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.

Uday Kiran

Comment 17 2012-05-14 23:11:32 PDT

Created attachment 141859 [details] Rebased Patch

Julien Chaffraix

Comment 18 2012-05-17 09:58:03 PDT

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.

Uday Kiran

Comment 19 2012-05-17 22:36:17 PDT

Created attachment 142632 [details] Updated Patch Fixed review comments and rebased.

Julien Chaffraix

Comment 20 2012-05-21 10:39:48 PDT

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 > +  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.

Uday Kiran

Comment 21 2012-05-25 03:33:16 PDT

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.

Uday Kiran

Comment 22 2012-05-25 03:51:28 PDT

Created attachment 144035 [details] Updated Patch changed to use ahem font.

Julien Chaffraix

Comment 23 2012-05-25 12:23:54 PDT

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 > + 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.

Uday Kiran

Comment 24 2012-05-27 21:55:31 PDT

Created attachment 144263 [details] Updated Patch

Uday Kiran

Comment 25 2012-05-27 22:04:53 PDT

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 >> + > > 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.

Kenneth Rohde Christiansen

Comment 26 2012-05-28 02:58:29 PDT

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?

Uday Kiran

Comment 27 2012-06-06 04:10:55 PDT

(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?

Ahmad Saleem

Comment 28 2023-05-08 14:11:45 PDT

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

Ahmad Saleem

Comment 29 2023-05-09 10:00:55 PDT

PR Attempt - https://github.com/WebKit/WebKit/pull/13637 Based on Uday's patch.

Ahmad Saleem

Comment 30 2023-05-09 10:44:51 PDT

(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

Radar WebKit Bug Importer

Comment 31 2024-02-08 15:43:47 PST

<rdar://problem/122588360>

Note You need to log in before you can comment on or make changes to this bug.