Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(24)

Issue 15841015: Fix a zooming bug with EM MQs (Closed)

Created:
7 years, 6 months ago by Yoav Weiss
Modified:
7 years, 6 months ago
CC:
blink-reviews, apavlov+blink_chromium.org, dglazkov+blink, eae+blinkwatch, kenneth.christiansen, darktears
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Fix a zooming bug with EM MQs BUG=245449 EM MQs were using calculated font size (which include the zooming factor) in order to translate the expected MQ size to CSS pixels. At the same time, adjustForAbsoluteZoom was taking the zoom into account to decrease the number of CSS pixels that fit into the viewport. That created a situation where the zoom is taken into account twice, causing a difference between EM MQs and PX MQs. That wasn't as bad as it could be because the calculated font size wasn't updating itself on every zoom step, but there was still visible difference. This change simply turns off font calculation for the CSS pixel computation, which results in parity between PX and EM MQs. I also added a layout test that covers this issue. Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152742

Patch Set 1 #

Total comments: 8

Patch Set 2 : Fixed style issues and test improvements according to review comments #

Patch Set 3 : Replaced previous tests with John Mellor's better ones #

Unified diffs Side-by-side diffs Delta from patch set Stats (+98 lines, -1 line) Patch
A LayoutTests/fast/css/zoom-media-queries.html View 1 2 1 chunk +60 lines, -0 lines 0 comments Download
A LayoutTests/fast/css/zoom-media-queries-expected.txt View 1 2 1 chunk +37 lines, -0 lines 0 comments Download
M Source/core/css/MediaQueryEvaluator.cpp View 1 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 12 (0 generated)
Yoav Weiss
I've found a bug with user zoom and "em" based MQs. A review of the ...
7 years, 6 months ago (2013-05-30 22:18:57 UTC) #1
abarth-chromium
I'd need to study the code you're changing more closely, but it seems like there's ...
7 years, 6 months ago (2013-05-31 08:11:21 UTC) #2
kenneth.r.christiansen
https://codereview.chromium.org/15841015/diff/1/LayoutTests/fast/css/zoom-media-queries-expected.txt File LayoutTests/fast/css/zoom-media-queries-expected.txt (right): https://codereview.chromium.org/15841015/diff/1/LayoutTests/fast/css/zoom-media-queries-expected.txt#newcode7 LayoutTests/fast/css/zoom-media-queries-expected.txt:7: CONSOLE MESSAGE: line 24: em:15.99392318725586px Something like shouldBe() or ...
7 years, 6 months ago (2013-05-31 08:21:21 UTC) #3
Yoav Weiss
I've updated the CL (Improved style and better test) according to review comments
7 years, 6 months ago (2013-05-31 13:28:01 UTC) #4
johnme
Code change looks good to me. This was definitely something that needed fixing (see also ...
7 years, 6 months ago (2013-06-18 18:06:47 UTC) #5
Yoav Weiss
I've replaced my previous tests with John Mellor's simpler & clearer tests.
7 years, 6 months ago (2013-06-18 21:57:09 UTC) #6
johnme
LGTM, thanks. You'll also need approval from an owner, such as abarth, before submitting.
7 years, 6 months ago (2013-06-19 11:13:19 UTC) #7
abarth-chromium
lgtm
7 years, 6 months ago (2013-06-19 17:39:36 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoav@yoav.ws/15841015/15001
7 years, 6 months ago (2013-06-19 17:39:46 UTC) #9
commit-bot: I haz the power
Step "update" is always a major failure. Look at the try server FAQ for more ...
7 years, 6 months ago (2013-06-19 17:47:38 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoav@yoav.ws/15841015/15001
7 years, 6 months ago (2013-06-19 18:24:04 UTC) #11
commit-bot: I haz the power
7 years, 6 months ago (2013-06-20 02:57:59 UTC) #12
Message was sent while issue was closed.
Change committed as 152742

Powered by Google App Engine
This is Rietveld 408576698