Bug 120114

Summary: [css3-text] Add editing test for CSS3 Text Decoration properties
Product: WebKit Reporter: Bruno Abinader <brunoabinader>
Component: CSSAssignee: Bruno Abinader <brunoabinader>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
darin: review+
Rebased patch for landing
none
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 none

Description Bruno Abinader 2013-08-21 09:04:18 PDT
This bug intends to add an editing layout test involving CSS3 Text Decoration properties, as requested in webkit-dev:
https://lists.webkit.org/pipermail/webkit-dev/2013-August/025304.html
Comment 1 Bruno Abinader 2013-08-21 09:09:00 PDT
Created attachment 209269 [details]
Patch
Comment 2 Ryosuke Niwa 2013-08-21 09:36:03 PDT
Comment on attachment 209269 [details]
Patch

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

> LayoutTests/ChangeLog:10
> +        Adds editing layout test to verify that CSS3 Text Decoration properties
> +        are being properly propagated. Skipping Mac, Qt and Win as they do not
> +        enable CSS3_TEXT feature flag by default.

We also need tests for copying & pasting a descendent of a node with text-decoration properties set since we ought to preserve the computed style in that case.
In addition, we need tests for toggling underline and strike through with these styles present as well as tests to merge paragraphs (i.e. deleting line breaks) with and without these styles.
We should also test what happens when the user types before/after or deletes text with these text decorations.

> LayoutTests/editing/pasteboard/insert-text-decoration.html:16
> +            document.querySelector('div[contenteditable]').focus();

Why don't we just do document.getElementById('container').focus() instead.
Btw, I hate indentations in this file.
Comment 3 Bruno Abinader 2013-08-29 14:34:32 PDT
(In reply to comment #2)
> (From update of attachment 209269 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209269&action=review
> 
> > LayoutTests/ChangeLog:10
> > +        Adds editing layout test to verify that CSS3 Text Decoration properties
> > +        are being properly propagated. Skipping Mac, Qt and Win as they do not
> > +        enable CSS3_TEXT feature flag by default.
> 
> We also need tests for copying & pasting a descendent of a node with text-decoration properties set since we ought to preserve the computed style in that case.
> In addition, we need tests for toggling underline and strike through with these styles present as well as tests to merge paragraphs (i.e. deleting line breaks) with and without these styles.
> We should also test what happens when the user types before/after or deletes text with these text decorations.

Ack. Regarding this, while implementing the toggling underline/strikethrough editing test I noticed an issue with mixed usage of text-decoration/-webkit-text-decoration-line. Currently, these two properties shares the same data internally, however, this confuses editing code. I'll create a new bug for this to be resolved separately.

> 
> > LayoutTests/editing/pasteboard/insert-text-decoration.html:16
> > +            document.querySelector('div[contenteditable]').focus();
> 
> Why don't we just do document.getElementById('container').focus() instead.
> Btw, I hate indentations in this file.

No problem. I was following Julien's recommendation on having indented code, but I can switch back to the  indentation style from other WebKit layout tests.
Comment 4 Darin Adler 2014-07-12 16:59:53 PDT
Comment on attachment 209269 [details]
Patch

Seems good to land some new tests. But this patch is really old, I doubt it will apply.
Comment 5 Bruno Abinader 2014-07-14 06:33:04 PDT
(In reply to comment #4)
> (From update of attachment 209269 [details])
> Seems good to land some new tests. But this patch is really old, I doubt it will apply.

Hi Darin, thanks for the update :) Sure, I can rebase them if necessary.
Comment 6 Bruno Abinader 2014-07-14 08:22:08 PDT
Created attachment 234853 [details]
Rebased patch for landing
Comment 7 Build Bot 2014-07-14 09:25:16 PDT
Comment on attachment 234853 [details]
Rebased patch for landing

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

New failing tests:
media/W3C/video/networkState/networkState_during_loadstart.html
Comment 8 Build Bot 2014-07-14 09:25:18 PDT
Created attachment 234858 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 9 Bruno Abinader 2014-07-14 09:45:30 PDT
Comment on attachment 234853 [details]
Rebased patch for landing

The failure in mac-wk2 is not related with this change.
Comment 10 WebKit Commit Bot 2014-07-14 10:17:24 PDT
Comment on attachment 234853 [details]
Rebased patch for landing

Clearing flags on attachment: 234853

Committed r171067: <http://trac.webkit.org/changeset/171067>