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.
Created attachment 175815 [details] css3-text-underline-position Implement 'alphabetical', 'under' and partially implement 'auto'
Created attachment 177785 [details] css3-text-underline-position Update to properly implement TextUnderlinePositionUnder
Created attachment 181828 [details] css3-text-underline-position (EWS version) Patch just for EWS
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
Created attachment 182145 [details] css3-text-underline-position (EWS version) Attempt to fix problem reported by gtk and efl bots
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
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
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
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
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
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/15766902
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
Comment on attachment 177785 [details] css3-text-underline-position New version will be added once it passes EWS check.
Created attachment 182255 [details] css3-text-underline-position (EWS version) Fixes all remaining known issues
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
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
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
Created attachment 182581 [details] Test Test bots
Comment on attachment 182581 [details] Test Attachment 182581 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15870555
Created attachment 183547 [details] css3-text-underline-position New patch with unit tests updated
Created attachment 183548 [details] css3-text-underline-position (EWS only version) Same patch with css3-text enabled by default
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.
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
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
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
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
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
Created attachment 183698 [details] css3-text-underline-position (EWS only version) Move .png files to proper place.
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.
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
(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?
Created attachment 184342 [details] css3-text-underline-position (EWS only version) Remove changes from StyleResolver::adjustRenderStyle(), this fixes the regressions reported by buildbots.
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.
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
Created attachment 184467 [details] css3-text-underline-position Remove changes from StyleResolver::adjustRenderStyle(), this fixes the regressions reported by buildbots.
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.
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
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
Created attachment 189755 [details] css3-text-underline-position Update unit tests.
Created attachment 189758 [details] css3-text-underline-position (EWS only version) Same patch with css3-text enabled.
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
Created attachment 189929 [details] css3-text-underline-position (EWS only version) Update parser because of changes in WebKit source code.
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
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
Created attachment 192863 [details] css3-text-underline-position Fix some code style issues in the patch.
Created attachment 192926 [details] css3-text-underline-position (EWS only version) Same patch with css3-text enabled.
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
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.
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.
(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.
(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.
(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.
Created attachment 193309 [details] css3-text-underline-position Fix issues reported.
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 > + <!-- Bugzilla link: http://webkit.org/b/102795 --> 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 > + <!-- Bugzilla link: http://webkit.org/b/102795 --> Ditto. > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-auto.html:4 > + <!-- Bugzilla link: http://webkit.org/b/102795 --> Ditto. > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-under-out-of-flow.html:4 > + <!-- Bugzilla link: http://webkit.org/b/102795 --> 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.
Created attachment 193624 [details] css3-text-underline-position Patch for landing
Comment on attachment 193624 [details] css3-text-underline-position Clearing flags on attachment: 193624 Committed r146104: <http://trac.webkit.org/changeset/146104>