Bug 102795

Summary:

[css3-text] Add rendering support for -webkit-text-underline-position

Product:

WebKit

Reporter:

Lamarque V. Souza <lamarque>

Component:

CSS

Assignee:

Lamarque V. Souza <lamarque>

Status:

RESOLVED FIXED

Severity:

Enhancement

CC:

abarth, abecsi, allan.jensen, bruno.abinader, buildbot, cmarcelo, dbates, dglazkov, eric, esprehn+autocc, gtk-ews, gyuyoung.kim, jchaffraix, leviw, macpherson, menard, ojan.autocc, peter+ews, philn, rakuco, rego+ews, rniwa, syoichi, vestbo, webkit-ews, webkit.review.bot, xan.lopez, zan

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

URL:

http://dev.w3.org/csswg/css-text-decor-3/#text-underline-position-property

Bug Depends on:

102491

Bug Blocks:

58491, 101931

Attachments:

Description	Flags
css3-text-underline-position	none
css3-text-underline-position	none
css3-text-underline-position (EWS version)	eflews.bot: commit-queue-
css3-text-underline-position (EWS version)	none
css3-text-underline-position (EWS version)	none
Test	none
css3-text-underline-position	none
css3-text-underline-position (EWS only version)	none
css3-text-underline-position (EWS only version)	none
css3-text-underline-position (EWS only version)	none
css3-text-underline-position	none
css3-text-underline-position	none
css3-text-underline-position (EWS only version)	none
css3-text-underline-position (EWS only version)	none
css3-text-underline-position	none
css3-text-underline-position (EWS only version)	none
css3-text-underline-position	leviw: review+
css3-text-underline-position	none

Lamarque V. Souza

Reported 2012-11-20 04:14:35 PST

This bug intends to add rendering support for "text-underline-position" CSS3 property. Parsing support is implemented on bug 102491. This is a sub-task for bug 101931.

Attachments
css3-text-underline-position (18.73 KB, patch) 2012-11-23 09:13 PST, Lamarque V. Souza	no flags	Details Formatted Diff Diff
css3-text-underline-position (23.72 KB, patch) 2012-12-05 10:07 PST, Lamarque V. Souza	no flags	Details Formatted Diff Diff
css3-text-underline-position (EWS version) (54.85 KB, patch) 2013-01-08 20:12 PST, Lamarque V. Souza	eflews.bot: commit-queue-	Details Formatted Diff Diff
css3-text-underline-position (EWS version) (63.44 KB, patch) 2013-01-10 08:17 PST, Lamarque V. Souza	no flags	Details Formatted Diff Diff
css3-text-underline-position (EWS version) (64.27 KB, patch) 2013-01-10 20:36 PST, Lamarque V. Souza	no flags	Details Formatted Diff Diff
Test (9.31 KB, patch) 2013-01-14 08:39 PST, Lamarque V. Souza	no flags	Details Formatted Diff Diff
css3-text-underline-position (176.97 KB, patch) 2013-01-18 14:31 PST, Lamarque V. Souza	no flags	Details Formatted Diff Diff
css3-text-underline-position (EWS only version) (221.96 KB, patch) 2013-01-18 14:35 PST, Lamarque V. Souza	no flags	Details Formatted Diff Diff
css3-text-underline-position (EWS only version) (222.66 KB, patch) 2013-01-20 19:19 PST, Lamarque V. Souza	no flags	Details Formatted Diff Diff
css3-text-underline-position (EWS only version) (222.16 KB, patch) 2013-01-23 16:37 PST, Lamarque V. Souza	no flags	Details Formatted Diff Diff
css3-text-underline-position (175.96 KB, patch) 2013-01-24 05:19 PST, Lamarque V. Souza	no flags	Details Formatted Diff Diff
css3-text-underline-position (75.21 KB, patch) 2013-02-22 05:36 PST, Lamarque V. Souza	no flags	Details Formatted Diff Diff
css3-text-underline-position (EWS only version) (122.06 KB, patch) 2013-02-22 05:42 PST, Lamarque V. Souza	no flags	Details Formatted Diff Diff
css3-text-underline-position (EWS only version) (121.36 KB, patch) 2013-02-23 09:26 PST, Lamarque V. Souza	no flags	Details Formatted Diff Diff
css3-text-underline-position (88.79 KB, patch) 2013-03-12 20:35 PDT, Lamarque V. Souza	no flags	Details Formatted Diff Diff
css3-text-underline-position (EWS only version) (103.27 KB, patch) 2013-03-13 08:15 PDT, Lamarque V. Souza	no flags	Details Formatted Diff Diff
css3-text-underline-position (105.96 KB, patch) 2013-03-15 07:52 PDT, Lamarque V. Souza	leviw: review+	Details Formatted Diff Diff
css3-text-underline-position (117.78 KB, patch) 2013-03-18 12:02 PDT, Lamarque V. Souza	no flags	Details Formatted Diff Diff
Show Obsolete (16) View All Add attachment proposed patch, testcase, etc.

Lamarque V. Souza

Comment 1 2012-11-23 09:13:51 PST

Created attachment 175815 [details] css3-text-underline-position Implement 'alphabetical', 'under' and partially implement 'auto'

Lamarque V. Souza

Comment 2 2012-12-05 10:07:05 PST

Created attachment 177785 [details] css3-text-underline-position Update to properly implement TextUnderlinePositionUnder

Lamarque V. Souza

Comment 3 2013-01-08 20:12:32 PST

Created attachment 181828 [details] css3-text-underline-position (EWS version) Patch just for EWS

EFL EWS Bot

Comment 4 2013-01-08 21:57:28 PST

Comment on attachment 181828 [details] css3-text-underline-position (EWS version) Attachment 181828 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15758406

Lamarque V. Souza

Comment 5 2013-01-10 08:17:58 PST

Created attachment 182145 [details] css3-text-underline-position (EWS version) Attempt to fix problem reported by gtk and efl bots

Early Warning System Bot

Comment 6 2013-01-10 08:31:49 PST

Comment on attachment 182145 [details] css3-text-underline-position (EWS version) Attachment 182145 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15759987

Early Warning System Bot

Comment 7 2013-01-10 08:32:38 PST

Comment on attachment 182145 [details] css3-text-underline-position (EWS version) Attachment 182145 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15763988

Build Bot

Comment 8 2013-01-10 09:04:26 PST

Comment on attachment 182145 [details] css3-text-underline-position (EWS version) Attachment 182145 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15795263

Peter Beverloo (cr-android ews)

Comment 9 2013-01-10 09:37:30 PST

Comment on attachment 182145 [details] css3-text-underline-position (EWS version) Attachment 182145 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15806005

WebKit Review Bot

Comment 10 2013-01-10 09:59:49 PST

Comment on attachment 182145 [details] css3-text-underline-position (EWS version) Attachment 182145 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15795271 New failing tests: fast/css-generated-content/content-property-change.html fast/css/content/content-quotes-03.html fast/css/content/content-quotes-01.html fast/css-generated-content/quotes-lang.html fast/css-generated-content/close-quote-negative-depth.html fast/css-generated-content/nested-quote-more-than-pairs.html fast/css-generated-content/quotes-lang-case-insensitive.html fast/css/content/content-quotes-05.html fast/css/content/content-quotes-02.html fast/css/content/content-quotes-04.html fast/css/content/content-quotes-06.html

Peter Beverloo (cr-android ews)

Comment 11 2013-01-10 10:38:05 PST

Build Bot

Comment 12 2013-01-10 11:56:48 PST

Comment on attachment 182145 [details] css3-text-underline-position (EWS version) Attachment 182145 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15760982 New failing tests: fast/css/content/content-quotes-03.html fast/css/content/content-quotes-01.html fast/css-generated-content/close-quote-negative-depth.html fast/css/content/content-quotes-04.html fast/css/content/content-quotes-05.html fast/css/content/content-quotes-02.html fast/css/content/content-quotes-06.html

Lamarque V. Souza

Comment 13 2013-01-10 12:15:56 PST

Comment on attachment 177785 [details] css3-text-underline-position New version will be added once it passes EWS check.

Lamarque V. Souza

Comment 14 2013-01-10 20:36:52 PST

Created attachment 182255 [details] css3-text-underline-position (EWS version) Fixes all remaining known issues

Build Bot

Comment 15 2013-01-10 22:25:34 PST

Comment on attachment 182255 [details] css3-text-underline-position (EWS version) Attachment 182255 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15812095 New failing tests: fast/css-generated-content/content-property-change.html fast/css/content/content-quotes-03.html fast/css/content/content-quotes-01.html fast/css-generated-content/quotes-lang.html fast/css-generated-content/close-quote-negative-depth.html fast/css-generated-content/nested-quote-more-than-pairs.html fast/css-generated-content/quotes-lang-case-insensitive.html fast/css/content/content-quotes-05.html fast/css/content/content-quotes-02.html fast/css/content/content-quotes-04.html fast/css/content/content-quotes-06.html

WebKit Review Bot

Comment 16 2013-01-10 22:31:00 PST

Comment on attachment 182255 [details] css3-text-underline-position (EWS version) Attachment 182255 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15795496 New failing tests: fast/css-generated-content/content-property-change.html fast/css/content/content-quotes-03.html fast/css/content/content-quotes-01.html fast/css-generated-content/quotes-lang.html fast/css-generated-content/close-quote-negative-depth.html fast/css-generated-content/nested-quote-more-than-pairs.html fast/css-generated-content/quotes-lang-case-insensitive.html fast/css/content/content-quotes-05.html fast/css/content/content-quotes-02.html fast/css/content/content-quotes-04.html fast/css/content/content-quotes-06.html

WebKit Review Bot

Comment 17 2013-01-10 23:28:27 PST

Comment on attachment 182255 [details] css3-text-underline-position (EWS version) Attachment 182255 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15810186 New failing tests: fast/css-generated-content/content-property-change.html fast/css/content/content-quotes-03.html fast/css/content/content-quotes-01.html fast/css-generated-content/quotes-lang.html fast/css-generated-content/close-quote-negative-depth.html fast/css-generated-content/nested-quote-more-than-pairs.html fast/css-generated-content/quotes-lang-case-insensitive.html fast/css/content/content-quotes-05.html fast/css/content/content-quotes-02.html fast/css/content/content-quotes-04.html fast/css/content/content-quotes-06.html

Lamarque V. Souza

Comment 18 2013-01-14 08:39:53 PST

Created attachment 182581 [details] Test Test bots

Build Bot

Comment 19 2013-01-14 08:45:05 PST

Comment on attachment 182581 [details] Test Attachment 182581 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15870555

Lamarque V. Souza

Comment 20 2013-01-18 14:31:23 PST

Created attachment 183547 [details] css3-text-underline-position New patch with unit tests updated

Lamarque V. Souza

Comment 21 2013-01-18 14:35:56 PST

Created attachment 183548 [details] css3-text-underline-position (EWS only version) Same patch with css3-text enabled by default

WebKit Review Bot

Comment 22 2013-01-18 14:40:20 PST

Attachment 183548 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic4-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5] Total errors found: 1 in 70 files If any of these errors are false positives, please file a bug against check-webkit-style.

WebKit Review Bot

Comment 23 2013-01-18 15:58:59 PST

Comment on attachment 183548 [details] css3-text-underline-position (EWS only version) Attachment 183548 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15955432 New failing tests: fast/css-generated-content/content-property-change.html fast/css/content/content-quotes-03.html fast/css/content/content-quotes-01.html fast/css-generated-content/quotes-lang.html fast/css-generated-content/close-quote-negative-depth.html fast/css-generated-content/nested-quote-more-than-pairs.html fast/css-generated-content/quotes-lang-case-insensitive.html fast/css/content/content-quotes-05.html fast/css/content/content-quotes-02.html fast/css/content/content-quotes-04.html fast/css/content/content-quotes-06.html

Build Bot

Comment 24 2013-01-18 16:56:03 PST

Comment on attachment 183548 [details] css3-text-underline-position (EWS only version) Attachment 183548 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15949528 New failing tests: fast/css-generated-content/content-property-change.html fast/css/content/content-quotes-03.html fast/css/content/content-quotes-01.html fast/css-generated-content/quotes-lang.html fast/css-generated-content/close-quote-negative-depth.html fast/css-generated-content/nested-quote-more-than-pairs.html fast/css-generated-content/quotes-lang-case-insensitive.html fast/css/content/content-quotes-05.html fast/css/content/content-quotes-02.html fast/css/content/content-quotes-04.html fast/css/content/content-quotes-06.html

Build Bot

Comment 25 2013-01-18 18:15:47 PST

Comment on attachment 183548 [details] css3-text-underline-position (EWS only version) Attachment 183548 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15937960 New failing tests: fast/css-generated-content/content-property-change.html fast/css/content/content-quotes-03.html fast/css/content/content-quotes-01.html fast/css-generated-content/quotes-lang.html fast/css-generated-content/close-quote-negative-depth.html fast/css-generated-content/nested-quote-more-than-pairs.html fast/css-generated-content/quotes-lang-case-insensitive.html fast/css/content/content-quotes-05.html fast/css/content/content-quotes-02.html fast/css/content/content-quotes-04.html fast/css/content/content-quotes-06.html

Build Bot

Comment 26 2013-01-18 18:28:46 PST

Comment on attachment 183548 [details] css3-text-underline-position (EWS only version) Attachment 183548 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15943803

Build Bot

Comment 27 2013-01-19 05:40:30 PST

Comment on attachment 183548 [details] css3-text-underline-position (EWS only version) Attachment 183548 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/15975194 New failing tests: fast/css-generated-content/content-property-change.html fast/css/content/content-quotes-03.html fast/css/content/content-quotes-01.html fast/css-generated-content/close-quote-negative-depth.html fast/css-generated-content/quotes-lang.html fast/css-generated-content/nested-quote-more-than-pairs.html fast/css-generated-content/quotes-lang-case-insensitive.html fast/css/content/content-quotes-05.html fast/css/content/content-quotes-02.html fast/css/content/content-quotes-04.html fast/css/content/content-quotes-06.html

Lamarque V. Souza

Comment 28 2013-01-20 19:19:30 PST

Created attachment 183698 [details] css3-text-underline-position (EWS only version) Move .png files to proper place.

WebKit Review Bot

Comment 29 2013-01-20 19:30:56 PST

Attachment 183698 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-auto2-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5] Total errors found: 1 in 70 files If any of these errors are false positives, please file a bug against check-webkit-style.

Build Bot

Comment 30 2013-01-20 20:03:44 PST

Comment on attachment 183698 [details] css3-text-underline-position (EWS only version) Attachment 183698 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15967831

Build Bot

Comment 31 2013-01-20 21:01:46 PST

Comment on attachment 183698 [details] css3-text-underline-position (EWS only version) Attachment 183698 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15968872

WebKit Review Bot

Comment 32 2013-01-20 21:09:28 PST

Comment on attachment 183698 [details] css3-text-underline-position (EWS only version) Attachment 183698 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16011126 New failing tests: fast/css-generated-content/content-property-change.html fast/css/content/content-quotes-03.html fast/css/content/content-quotes-01.html fast/css-generated-content/quotes-lang.html fast/css-generated-content/close-quote-negative-depth.html fast/css-generated-content/nested-quote-more-than-pairs.html fast/css-generated-content/quotes-lang-case-insensitive.html fast/css/content/content-quotes-05.html fast/css/content/content-quotes-02.html fast/css/content/content-quotes-04.html fast/css/content/content-quotes-06.html

Build Bot

Comment 33 2013-01-20 22:05:33 PST

Comment on attachment 183698 [details] css3-text-underline-position (EWS only version) Attachment 183698 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15967863 New failing tests: fast/css-generated-content/content-property-change.html fast/css/content/content-quotes-03.html fast/css/content/content-quotes-01.html fast/css-generated-content/quotes-lang.html fast/css-generated-content/close-quote-negative-depth.html fast/css-generated-content/nested-quote-more-than-pairs.html fast/css-generated-content/quotes-lang-case-insensitive.html fast/css/content/content-quotes-05.html fast/css/content/content-quotes-02.html fast/css/content/content-quotes-04.html fast/css/content/content-quotes-06.html

Build Bot

Comment 34 2013-01-20 22:05:40 PST

Comment on attachment 183698 [details] css3-text-underline-position (EWS only version) Attachment 183698 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15968895

WebKit Review Bot

Comment 35 2013-01-20 22:13:44 PST

Comment on attachment 183698 [details] css3-text-underline-position (EWS only version) Attachment 183698 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15974876 New failing tests: fast/css-generated-content/content-property-change.html fast/css/content/content-quotes-03.html fast/css/content/content-quotes-01.html fast/css-generated-content/quotes-lang.html fast/css-generated-content/close-quote-negative-depth.html fast/css-generated-content/nested-quote-more-than-pairs.html fast/css-generated-content/quotes-lang-case-insensitive.html fast/css/content/content-quotes-05.html fast/css/content/content-quotes-02.html fast/css/content/content-quotes-04.html fast/css/content/content-quotes-06.html

Build Bot

Comment 36 2013-01-20 22:27:06 PST

Comment on attachment 183698 [details] css3-text-underline-position (EWS only version) Attachment 183698 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/15975926 New failing tests: fast/css-generated-content/content-property-change.html fast/css/content/content-quotes-03.html fast/css/content/content-quotes-01.html fast/css-generated-content/close-quote-negative-depth.html fast/css-generated-content/quotes-lang.html fast/css-generated-content/nested-quote-more-than-pairs.html fast/css-generated-content/quotes-lang-case-insensitive.html fast/css/content/content-quotes-05.html fast/css/content/content-quotes-02.html fast/css/content/content-quotes-04.html fast/css/content/content-quotes-06.html

Build Bot

Comment 37 2013-01-20 23:09:15 PST

Comment on attachment 183698 [details] css3-text-underline-position (EWS only version) Attachment 183698 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15974902

Build Bot

Comment 38 2013-01-20 23:19:51 PST

Comment on attachment 183698 [details] css3-text-underline-position (EWS only version) Attachment 183698 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15968917 New failing tests: fast/css-generated-content/content-property-change.html fast/css/content/content-quotes-03.html fast/css/content/content-quotes-01.html fast/css-generated-content/quotes-lang.html fast/css-generated-content/close-quote-negative-depth.html fast/css-generated-content/nested-quote-more-than-pairs.html fast/css-generated-content/quotes-lang-case-insensitive.html fast/css/content/content-quotes-05.html fast/css/content/content-quotes-02.html fast/css/content/content-quotes-04.html fast/css/content/content-quotes-06.html

WebKit Review Bot

Comment 39 2013-01-21 00:14:34 PST

Comment on attachment 183698 [details] css3-text-underline-position (EWS only version) Attachment 183698 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15975964 New failing tests: fast/css-generated-content/content-property-change.html fast/css/content/content-quotes-03.html fast/css/content/content-quotes-01.html fast/css-generated-content/quotes-lang.html fast/css-generated-content/close-quote-negative-depth.html fast/css-generated-content/nested-quote-more-than-pairs.html fast/css-generated-content/quotes-lang-case-insensitive.html fast/css/content/content-quotes-05.html fast/css/content/content-quotes-02.html fast/css/content/content-quotes-04.html fast/css/content/content-quotes-06.html

Build Bot

Comment 40 2013-01-21 00:16:01 PST

Comment on attachment 183698 [details] css3-text-underline-position (EWS only version) Attachment 183698 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15977901 New failing tests: fast/css-generated-content/content-property-change.html fast/css/content/content-quotes-03.html fast/css/content/content-quotes-01.html fast/css-generated-content/quotes-lang.html fast/css-generated-content/close-quote-negative-depth.html fast/css-generated-content/nested-quote-more-than-pairs.html fast/css-generated-content/quotes-lang-case-insensitive.html fast/css/content/content-quotes-05.html fast/css/content/content-quotes-02.html fast/css/content/content-quotes-04.html fast/css/content/content-quotes-06.html

Build Bot

Comment 41 2013-01-21 00:16:45 PST

Comment on attachment 183698 [details] css3-text-underline-position (EWS only version) Attachment 183698 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16011194

Build Bot

Comment 42 2013-01-21 00:28:07 PST

Comment on attachment 183698 [details] css3-text-underline-position (EWS only version) Attachment 183698 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/15975972 New failing tests: fast/css-generated-content/content-property-change.html fast/css/content/content-quotes-03.html fast/css/content/content-quotes-01.html fast/css-generated-content/close-quote-negative-depth.html fast/css-generated-content/quotes-lang.html fast/css-generated-content/nested-quote-more-than-pairs.html fast/css-generated-content/quotes-lang-case-insensitive.html fast/css/content/content-quotes-05.html fast/css/content/content-quotes-02.html fast/css/content/content-quotes-04.html fast/css/content/content-quotes-06.html

Build Bot

Comment 43 2013-01-21 01:15:03 PST

Comment on attachment 183698 [details] css3-text-underline-position (EWS only version) Attachment 183698 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15974934

WebKit Review Bot

Comment 44 2013-01-21 05:15:15 PST

Comment on attachment 183698 [details] css3-text-underline-position (EWS only version) Attachment 183698 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16031055 New failing tests: fast/css-generated-content/content-property-change.html fast/css/content/content-quotes-03.html fast/css/content/content-quotes-01.html fast/css-generated-content/quotes-lang.html fast/css-generated-content/close-quote-negative-depth.html fast/css-generated-content/nested-quote-more-than-pairs.html fast/css-generated-content/quotes-lang-case-insensitive.html fast/css/content/content-quotes-05.html fast/css/content/content-quotes-02.html fast/css/content/content-quotes-04.html fast/css/content/content-quotes-06.html

WebKit Review Bot

Comment 45 2013-01-21 06:14:17 PST

Comment on attachment 183698 [details] css3-text-underline-position (EWS only version) Attachment 183698 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16033084 New failing tests: fast/css-generated-content/content-property-change.html fast/css/content/content-quotes-03.html fast/css/content/content-quotes-01.html fast/css-generated-content/quotes-lang.html fast/css-generated-content/close-quote-negative-depth.html fast/css-generated-content/nested-quote-more-than-pairs.html fast/css-generated-content/quotes-lang-case-insensitive.html fast/css/content/content-quotes-05.html fast/css/content/content-quotes-02.html fast/css/content/content-quotes-04.html fast/css/content/content-quotes-06.html

Lamarque V. Souza

Comment 46 2013-01-23 11:46:07 PST

(In reply to comment #45) > (From update of attachment 183698 [details]) > Attachment 183698 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/16033084 > > New failing tests: > fast/css-generated-content/content-property-change.html > fast/css/content/content-quotes-03.html > fast/css/content/content-quotes-01.html > fast/css-generated-content/quotes-lang.html > fast/css-generated-content/close-quote-negative-depth.html > fast/css-generated-content/nested-quote-more-than-pairs.html > fast/css-generated-content/quotes-lang-case-insensitive.html > fast/css/content/content-quotes-05.html > fast/css/content/content-quotes-02.html > fast/css/content/content-quotes-04.html > fast/css/content/content-quotes-06.html I have installed a chrooted Ubuntu installation (Lucid64) and none of those unit tests fail here. Anyone have any clue on what could be happening?

Lamarque V. Souza

Comment 47 2013-01-23 16:37:04 PST

Created attachment 184342 [details] css3-text-underline-position (EWS only version) Remove changes from StyleResolver::adjustRenderStyle(), this fixes the regressions reported by buildbots.

WebKit Review Bot

Comment 48 2013-01-23 16:42:15 PST

Attachment 184342 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-underline-position-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-underline-position.html', u'LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-underline-position.js', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic1-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic1.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic2-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic2.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic3-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic3.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic4-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic4.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic5-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic5.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto1-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto1.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto2-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto2.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto3-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto3.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto4-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto4.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto5-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto5.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under1-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under1.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under2-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under2.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under3-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under3.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under4-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under4.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under5-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under5.html', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic1-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic2-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic3-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic4-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic5-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-auto1-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-auto2-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-auto3-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-auto4-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-auto5-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-under1-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-under2-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-under3-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-under4-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-under5-expected.png', u'LayoutTests/platform/chromium/TestExpectations', u'Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig', u'Source/WebCore/ChangeLog', u'Source/WebCore/Configurations/FeatureDefines.xcconfig', u'Source/WebCore/GNUmakefile.features.am.in', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSGrammar.y.in', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSParser.h', u'Source/WebCore/css/CSSPrimitiveValueMappings.h', u'Source/WebCore/css/CSSProperty.cpp', u'Source/WebCore/css/CSSPropertyNames.in', u'Source/WebCore/css/CSSRule.idl', u'Source/WebCore/css/CSSValueKeywords.in', u'Source/WebCore/css/StyleBuilder.cpp', u'Source/WebCore/rendering/InlineFlowBox.cpp', u'Source/WebCore/rendering/InlineFlowBox.h', u'Source/WebCore/rendering/InlineTextBox.cpp', u'Source/WebCore/rendering/RootInlineBox.cpp', u'Source/WebCore/rendering/RootInlineBox.h', u'Source/WebCore/rendering/style/RenderStyle.h', u'Source/WebCore/rendering/style/RenderStyleConstants.h', u'Source/WebCore/rendering/style/StyleRareInheritedData.cpp', u'Source/WebCore/rendering/style/StyleRareInheritedData.h', u'Source/WebKit/chromium/features.gypi', u'Source/WebKit/mac/Configurations/FeatureDefines.xcconfig', u'Source/WebKit2/Configurations/FeatureDefines.xcconfig', u'Source/cmake/OptionsCommon.cmake', u'Source/cmake/WebKitFeatures.cmake', u'Tools/Scripts/webkitperl/FeatureList.pm', u'Tools/qmake/mkspecs/features/features.pri', u'WebKitLibraries/win/tools/vsprops/FeatureDefines.vsprops', u'WebKitLibraries/win/tools/vsprops/FeatureDefinesCairo.vsprops', u'configure.ac']" exit_code: 1 LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-auto2-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5] Total errors found: 1 in 69 files If any of these errors are false positives, please file a bug against check-webkit-style.

Build Bot

Comment 49 2013-01-23 20:22:42 PST

Comment on attachment 184342 [details] css3-text-underline-position (EWS only version) Attachment 184342 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16084453

Lamarque V. Souza

Comment 50 2013-01-24 05:19:09 PST

Created attachment 184467 [details] css3-text-underline-position Remove changes from StyleResolver::adjustRenderStyle(), this fixes the regressions reported by buildbots.

WebKit Review Bot

Comment 51 2013-01-24 05:53:49 PST

Attachment 184467 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic1-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic1.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic2-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic2.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic3-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic3.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic4-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic4.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic5-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic5.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto1-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto1.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto2-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto2.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto3-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto3.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto4-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto4.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto5-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto5.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under1-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under1.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under2-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under2.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under3-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under3.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under4-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under4.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under5-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under5.html', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic1-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic2-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic3-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic4-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic5-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-auto1-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-auto2-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-auto3-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-auto4-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-auto5-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-under1-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-under2-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-under3-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-under4-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-under5-expected.png', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/InlineFlowBox.cpp', u'Source/WebCore/rendering/InlineFlowBox.h', u'Source/WebCore/rendering/InlineTextBox.cpp', u'Source/WebCore/rendering/RootInlineBox.cpp', u'Source/WebCore/rendering/RootInlineBox.h']" exit_code: 1 LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-auto2-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5] Total errors found: 1 in 38 files If any of these errors are false positives, please file a bug against check-webkit-style.

EFL EWS Bot

Comment 52 2013-01-24 06:48:08 PST

Comment on attachment 184467 [details] css3-text-underline-position Attachment 184467 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16083676

kov's GTK+ EWS bot

Comment 53 2013-01-24 07:22:05 PST

Comment on attachment 184467 [details] css3-text-underline-position Attachment 184467 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/16078733

Lamarque V. Souza

Comment 54 2013-02-22 05:36:58 PST

Created attachment 189755 [details] css3-text-underline-position Update unit tests.

Lamarque V. Souza

Comment 55 2013-02-22 05:42:21 PST

Created attachment 189758 [details] css3-text-underline-position (EWS only version) Same patch with css3-text enabled.

EFL EWS Bot

Comment 56 2013-02-22 06:01:48 PST

Comment on attachment 189755 [details] css3-text-underline-position Attachment 189755 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16700811

Lamarque V. Souza

Comment 57 2013-02-23 09:26:28 PST

Created attachment 189929 [details] css3-text-underline-position (EWS only version) Update parser because of changes in WebKit source code.

Build Bot

Comment 58 2013-02-25 15:07:07 PST

Comment on attachment 189929 [details] css3-text-underline-position (EWS only version) Attachment 189929 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16744545

Peter Beverloo (cr-android ews)

Comment 59 2013-02-27 02:06:22 PST

Comment on attachment 189929 [details] css3-text-underline-position (EWS only version) Attachment 189929 [details] did not pass cr-android-ews (chromium-android): Output: http://webkit-commit-queue.appspot.com/results/16672190

Lamarque V. Souza

Comment 60 2013-03-12 20:35:06 PDT

Created attachment 192863 [details] css3-text-underline-position Fix some code style issues in the patch.

Lamarque V. Souza

Comment 61 2013-03-13 08:15:16 PDT

Created attachment 192926 [details] css3-text-underline-position (EWS only version) Same patch with css3-text enabled.

Build Bot

Comment 62 2013-03-13 09:38:40 PDT

Comment on attachment 192926 [details] css3-text-underline-position (EWS only version) Attachment 192926 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17184026

Julien Chaffraix

Comment 63 2013-03-13 13:49:13 PDT

Comment on attachment 192863 [details] css3-text-underline-position View in context: https://bugs.webkit.org/attachment.cgi?id=192863&action=review I don't know much about line layout so someone (e.g. levi!) should chime in on that. > Source/WebCore/ChangeLog:42 > + * rendering/style/RenderStyle.h: Please fill in the ChangeLog with useful information. > Source/WebCore/rendering/InlineFlowBox.cpp:747 > + if (curr->renderer()->isOutOfFlowPositioned()) > + continue; // Positioned placeholders don't affect calculations. This doesn't seem to be tested. > Source/WebCore/rendering/InlineTextBox.cpp:974 > + // FIXME: implement 'under left' and 'under right' and make TextUnderlinePositionAuto default to 'under left' > + // for vertical text. Do we really need to repeat that we miss 'left|right' everywhere? > Source/WebCore/rendering/InlineTextBox.cpp:983 > + default: We don't expect any value so I wouldn't put a 'default' as the compiler would be then able to catch any people forgetting to update this one. The code in the default would be moved just out of the switch. > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/script.js:2 > +if (window.testRunner) > + testRunner.dumpAsText(true); This would be better inlined for clarity in the tests as it doesn't really save much per test. > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/style.css:1 > +body { font: 14px Ahem; -webkit-font-smoothing: none; -webkit-text-stroke: 1px black; -webkit-text-fill-color: white; } Why not 10px? This code is very difficult to read as all the rules are on one line. > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-alphabetic2.html:1 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> Unless this will be submitted to the official test suite, please use the HTML5 doctype: <!DOCTYPE HTML> All the comments on this file apply to all the other test cases but I won't repeat them. > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-alphabetic2.html:7 > + <title>CSS Test: CSS3 text-underline-position</title> > + <link rel="help" href="http://dev.w3.org/csswg/css-text-decor-3/#text-underline-position-property"/> > + <meta name="flags" content="ahem"/> I found these indications not very helpful as they are not dumped in the output. If it's important enough, it should be dumped. If not, I think it should not be included. > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-alphabetic2.html:11 > + <body> You are missing 2 important facts in this test case: * What are you testing? (a 'description' of the test case) * What are the condition for passing? Ideally this should be a simple check (like 'there should be no red', 'you should see a 96px * 96px square'). See http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree about how to write better test cases.

Levi Weintraub

Comment 64 2013-03-13 14:05:32 PDT

Comment on attachment 192863 [details] css3-text-underline-position View in context: https://bugs.webkit.org/attachment.cgi?id=192863&action=review > Source/WebCore/rendering/InlineFlowBox.cpp:750 > + if (descendantsHaveSameLineHeightAndBaseline()) > + continue; This seems wrong. Shouldn't we still max maxLogicalTop with our curr->y() and just not recurse in this case? 0 is passed in initially, but just because this property is true doesn't mean we want to return 0, right? > Source/WebCore/rendering/InlineFlowBox.h:325 > + void computeMaxLogicalTops(float& maxLogicalTop); We're not computing multiple logical tops, we're computing the one max. I'd use computeMaxLogicalTop.

Lamarque V. Souza

Comment 65 2013-03-13 18:31:53 PDT

(In reply to comment #63) > Please fill in the ChangeLog with useful information. > > > Source/WebCore/rendering/InlineFlowBox.cpp:747 > > + if (curr->renderer()->isOutOfFlowPositioned()) > > + continue; // Positioned placeholders don't affect calculations. > > This doesn't seem to be tested. I do not follow. Do you mean curr->renderer()->isOutOfFlowPositioned() always returns the same value? I created computeMaxLogicalTop() based on InlineFlowBox::computeLogicalBoxHeights() and added this test because it and several other methods do the same. > > Source/WebCore/rendering/InlineTextBox.cpp:974 > > + // FIXME: implement 'under left' and 'under right' and make TextUnderlinePositionAuto default to 'under left' > > + // for vertical text. > > Do we really need to repeat that we miss 'left|right' everywhere? Well, this comment is important because we will need to change how TextUnderlinePositionAuto works to match the specification once 'under left' is implemented. What about change the text to "FIXME: According to the specification TextUnderlinePositionAuto should default to 'under left' for vertical text. Implement that once 'under left' is implemented."? TextUnderlinePositionAuto should default to 'alphabetic' for horizontal text according to the specification. > > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/style.css:1 > > +body { font: 14px Ahem; -webkit-font-smoothing: none; -webkit-text-stroke: 1px black; -webkit-text-fill-color: white; } > > Why not 10px? This code is very difficult to read as all the rules are on one line. No reason, actually the font size is not important. > > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-alphabetic2.html:7 > > + <title>CSS Test: CSS3 text-underline-position</title> > > + <link rel="help" href="http://dev.w3.org/csswg/css-text-decor-3/#text-underline-position-property"/> > > + <meta name="flags" content="ahem"/> > > I found these indications not very helpful as they are not dumped in the output. > > If it's important enough, it should be dumped. If not, I think it should not be included. I will remove them. All those indications can be found in the bug report too. > > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-alphabetic2.html:11 > > + <body> > > You are missing 2 important facts in this test case: > * What are you testing? (a 'description' of the test case) > * What are the condition for passing? Ideally this should be a simple check (like 'there should be no red', 'you should see a 96px * 96px square'). The tests checks at which offset the horizontal lines are placed. There are two possible positions: at the font's baseline plus a offset, which is usually 1 pixel, or at the font box's bottom edge plus the same 1 pixel offset. "text-underline-position: auto" and "alphabetic" use the first option, which is simple to implement. "text-underline-position: under" is more problematic to implement because InlineTextBox::paintDecoration() draws the decoration relative current InlineBox's top edge. I created computeMaxLogicalTop() so I can get the maximum offset and then, in computeUnderlineOffset(), calculate the offset I need to add to which InlineBox's y() so it matches the position of the lower InlineBox (the one with maxLogicalTop). computeMaxLogicalTop() and computeUnderlineOffset() are basically used to translate relative y() coordinate to global y() coordiate. It is difficult to see what the correct result is when using Ahem font, I used it because it is what WebKit recommends to use in unit tests, right? Do you think I can use another font here? Some of the tests mixes different text-underline-position values to test if the code correctly handles that cases. vertical-align also affects "text-underline-position: under", so it is also tested. I will add the respective description to each test so they can be understood.

Lamarque V. Souza

Comment 66 2013-03-13 18:39:03 PDT

(In reply to comment #64) > (From update of attachment 192863 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=192863&action=review > > > Source/WebCore/rendering/InlineFlowBox.cpp:750 > > + if (descendantsHaveSameLineHeightAndBaseline()) > > + continue; > > This seems wrong. Shouldn't we still max maxLogicalTop with our curr->y() and just not recurse in this case? 0 is passed in initially, but just because this property is true doesn't mean we want to return 0, right? 0 is a valid value for maxLogicalTop. If all InlineBox are aligned at the top of the page they all have y() == 0 and maxLogicalTop is 0. I need the recursion because curr()'s children must also be checked to calculate it, otherwise if one of its children is not aligned to the top of the parent (e.g. using "vertical-align: middle" like in text-underline-position-under5.html) it will have a higher y() than the parent and that must be counted according the text-underline-position's specification.

Julien Chaffraix

Comment 67 2013-03-14 08:04:32 PDT

(In the future, it's better to use the 'Review Patch' link to post side-by-side comments, our reviewing tool is fairly limited and wouldn't include your new comments). > > > Source/WebCore/rendering/InlineFlowBox.cpp:747 > > > + if (curr->renderer()->isOutOfFlowPositioned()) > > > + continue; // Positioned placeholders don't affect calculations. > > > > This doesn't seem to be tested. > > I do not follow. Do you mean curr->renderer()->isOutOfFlowPositioned() always returns the same value? I created computeMaxLogicalTop() based on InlineFlowBox::computeLogicalBoxHeights() and added this test because it and several other methods do the same. No, I merely talked about testing. There is no test with an out-of-flow positions element (postion: absolute or fixed) so this line is *definitely* not tested in any of them.

Lamarque V. Souza

Comment 68 2013-03-15 07:52:59 PDT

Created attachment 193309 [details] css3-text-underline-position Fix issues reported.

Levi Weintraub

Comment 69 2013-03-18 11:28:38 PDT

Comment on attachment 193309 [details] css3-text-underline-position View in context: https://bugs.webkit.org/attachment.cgi?id=193309&action=review The pixel tests make me sad, but it seems like they're necessary. > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-all.html:4 > +  I'd rather see this in the description than in a comment in the test. > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-alphabetic.html:4 > +  Ditto. > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-auto.html:4 > +  Ditto. > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-under-out-of-flow.html:4 > +  Ditto. > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-under-out-of-flow.html:12 > + <p>Test if out of flow boxes are ignored when calculating the offset for 'text-underline-position: under'. The horizontal line below should not go outside the box.</p> Nit observation: Strangely long line when you wrapped the other descriptions.

Lamarque V. Souza

Comment 70 2013-03-18 12:02:09 PDT

Created attachment 193624 [details] css3-text-underline-position Patch for landing

WebKit Review Bot

Comment 71 2013-03-18 12:45:47 PDT

Comment on attachment 193624 [details] css3-text-underline-position Clearing flags on attachment: 193624 Committed r146104: <http://trac.webkit.org/changeset/146104>

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