Bug 134651

Summary: Subpixel layout: return integral results for offset*, client*, scroll* by default.
Product: WebKit Reporter: zalan <zalan>
Component: Layout and RenderingAssignee: zalan <zalan>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cmarcelo, commit-queue, esprehn+autocc, kangil.han, rniwa, simon.fraser
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
none
Patch
none
Patch none

Description zalan 2014-07-05 09:12:39 PDT
Keep the double return type but round/truncate the return value. (see bug 132895)
<rdar://problem/17435776>

Fractional values break number of sites/JS frameworks:
1. gmail.com (inbox items don’t show up when certain languages are set), 
2. "Setup Instructions" on icloud.com ellipsized -> tight design. <rdar://problem/17028563>
3. can't into fidelity.netbenefits.com -> RequireJS fails to make some content fit due to offsetWidth (fractional) vs. scrollWidth (integral) value compare. <rdar://problem/17227701>
4. Text labels are truncated with elipses @ Find My iPhone @ icloud.com  -> similar to 2, tight design. <rdar://problem/17338131>
5. Table is missing from a page at bethematch.org  -> regex fails to handle fractional values (/.*if_height=(\d+)(?:&|$)/) <rdar://problem/17480566>
Comment 1 zalan 2014-07-05 09:55:17 PDT
1. FireFox: (integral values, no fractional support)
- FireFox has been advocating Element.getBoundingClientRect(1) and getBoxQuads(2) to access to fractional values instead of Element.offset* etc.

2. IE: (integral values by default, opt-in for fractional values)
- IE has the fractional behavior behind document.msCSSOMElementFloatMetrics property.(3)
- It’s off by default -> integral values (4).

3. Chrome (integral values by default for now)
- Chrome had been returning fractional values (5) by default until recently. 
- Chrome turned fractional values off in r176545 (6): -> "Due to breaking a number of high profile sites we've been forced to  revert this change. I plan to try again in M38. “.

(1) https://bugzilla.mozilla.org/show_bug.cgi?id=825607
(2) https://hacks.mozilla.org/2014/03/introducing-the-getboxquads-api/
(3) http://blogs.msdn.com/b/ie/archive/2012/02/17/sub-pixel-rendering-and-the-css-object-model.aspx?Redirected=true
(4) http://msdn.microsoft.com/en-us/library/ie/hh673543(v=vs.85).aspx
(5) https://src.chromium.org/viewvc/blink?revision=173426&view=revision
(6) http://src.chromium.org/viewvc/blink?view=revision&revision=176545
Comment 2 zalan 2014-07-05 10:09:27 PDT
Created attachment 234440 [details]
Patch
Comment 3 Build Bot 2014-07-05 11:40:03 PDT
Comment on attachment 234440 [details]
Patch

Attachment 234440 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5563074249490432

New failing tests:
editing/selection/drag-start-event-client-x-y.html
fast/block/positioning/offsetLeft-offsetTop-multicolumn.html
editing/selection/mixed-editability-10.html
editing/selection/collapse-selection-in-bidi.html
Comment 4 Build Bot 2014-07-05 11:40:06 PDT
Created attachment 234443 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 5 Build Bot 2014-07-05 12:15:07 PDT
Comment on attachment 234440 [details]
Patch

Attachment 234440 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5484350183309312

New failing tests:
editing/selection/drag-start-event-client-x-y.html
fast/block/positioning/offsetLeft-offsetTop-multicolumn.html
editing/selection/collapse-selection-in-bidi.html
Comment 6 Build Bot 2014-07-05 12:15:11 PDT
Created attachment 234446 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 7 Build Bot 2014-07-05 12:47:23 PDT
Comment on attachment 234440 [details]
Patch

Attachment 234440 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4681424300933120

New failing tests:
editing/selection/drag-start-event-client-x-y.html
fast/block/positioning/offsetLeft-offsetTop-multicolumn.html
editing/selection/mixed-editability-10.html
editing/selection/collapse-selection-in-bidi.html
Comment 8 Build Bot 2014-07-05 12:47:25 PDT
Created attachment 234447 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 9 Darin Adler 2014-07-06 17:19:44 PDT
Comment on attachment 234440 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234440&action=review

> Source/WebCore/ChangeLog:3
> +        Subpixel layout: return integral results for offset*, client*, scroll* by default.

Longer term, why are we even keeping this boolean setting? Seems we should not be paying any runtime overhead for this experiment. We now know that the non-integral values will need to come from other APIs.
Comment 10 zalan 2014-07-07 14:00:45 PDT
(In reply to comment #9)
> (From update of attachment 234440 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234440&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Subpixel layout: return integral results for offset*, client*, scroll* by default.
> 
> Longer term, why are we even keeping this boolean setting? Seems we should not be paying any runtime overhead for this experiment. We now know that the non-integral values will need to come from other APIs.
We need to keep this runtime setting around until after we introduced those new API as there's a Mac app that depends on these fractional values.
Comment 11 zalan 2014-07-08 14:23:19 PDT
Created attachment 234596 [details]
Patch
Comment 12 Simon Fraser (smfr) 2014-07-10 10:34:25 PDT
Comment on attachment 234596 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234596&action=review

> Source/WebCore/dom/Element.cpp:767
> +        double clientTop = subpixelMetricsEnabled(renderer->document()) ? renderer->clientTop().toDouble() : roundToInt(renderer->clientTop());

double -> LayoutUnit for all of these.
Comment 13 zalan 2014-07-11 07:49:16 PDT
Created attachment 234763 [details]
Patch
Comment 14 WebKit Commit Bot 2014-07-11 08:28:05 PDT
Comment on attachment 234763 [details]
Patch

Clearing flags on attachment: 234763

Committed r171001: <http://trac.webkit.org/changeset/171001>
Comment 15 WebKit Commit Bot 2014-07-11 08:28:10 PDT
All reviewed patches have been landed.  Closing bug.