Bug 120114 - [css3-text] Add editing test for CSS3 Text Decoration properties
Summary: [css3-text] Add editing test for CSS3 Text Decoration properties
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Bruno Abinader
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-21 09:04 PDT by Bruno Abinader
Modified: 2014-07-14 10:32 PDT (History)
3 users (show)

See Also:


Attachments
Patch (5.68 KB, patch)
2013-08-21 09:09 PDT, Bruno Abinader
darin: review+
Details | Formatted Diff | Diff
Rebased patch for landing (4.82 KB, patch)
2014-07-14 08:22 PDT, Bruno Abinader
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (607.27 KB, application/zip)
2014-07-14 09:25 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
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>