Bug 5631 - KWQKHTMLPart::attributedString ignores many tags
Summary: KWQKHTMLPart::attributedString ignores many tags
Status: VERIFIED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-11-04 14:36 PST by Alexey Proskuryakov
Modified: 2005-12-19 04:25 PST (History)
1 user (show)

See Also:


Attachments
proposed patch (6.18 KB, patch)
2005-11-04 14:36 PST, Alexey Proskuryakov
eric: review+
Details | Formatted Diff | Diff
patch with ignore-whitespace (1.10 KB, patch)
2005-11-28 21:24 PST, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2005-11-04 14:36:28 PST
Because of what seems to be incorrect if/else nesting, KWQKHTMLPart::attributedString doesn't handle ol, 
ul, td, th, hr, dd, dl, dt, pre, blockquote, div, p, tr, h1, h2, h3, h4, h5, h6 and img tags.

I do not know how to reproduce the problem with shipping WebKit or ToT, but I have tested the fix (for 
DIV elements) with the proposed implementation of attributedSubstringFromRange (bug 4680) and my 
input method.
Comment 1 Alexey Proskuryakov 2005-11-04 14:36:53 PST
Created attachment 4601 [details]
proposed patch
Comment 2 Darin Adler 2005-11-04 14:51:22 PST
Comment on attachment 4601 [details]
proposed patch

The concept in Tiger was to get out of the business of making attributed
strings. AppKit was supposed to handle that for us. The only reason this code
is still around is that it was needed on Panther.

If we are going to keep and enhance this code that's inside WebCore (which I'm
not sure is a good idea), then we need to change it so that the text part of it
comes from the text iterator, so the text part of the attributed string will
match the plain text in the plain text string.

None of that really tells us whether or not to take this particular patch,
which might be an OK incremental improvement.
Comment 3 Evan Gross 2005-11-04 19:38:53 PST
(In reply to comment #2)
Just to muddy the attributed string waters a little more - take a look at what's in KWQAccObject.mm. 
Yet another way of creating attributed strings. Granted, the names of the attributes are different for AX 
objects, but same basic idea.

So attributed strings are currently created in 3 different ways (and counting?).

Maybe time to take another look at the "secret" one that's in AppKit? Do we know *anything* about 
what it can and can't do?

Anyway, playing with this patch, I don't see that it solves the problem with ranges that end with a line 
break that come from converting an NSRange to a DOMRange. Not sure if that's because of bug 5610 (I 
think it must be), or something other problem in KWQKHTMLPart::attributedString.
Comment 4 Alexey Proskuryakov 2005-11-05 00:38:09 PST
However, this specific patch only fixes what seems to be a simple typo :)

Yes, there are still problems with ranges that end with a newline, but at least newlines at other positions  
work better with it.
Comment 5 Alexey Proskuryakov 2005-11-28 21:24:30 PST
Created attachment 4841 [details]
patch with ignore-whitespace

Same patch made with cvs diff -ub for easier reading
Comment 6 Eric Seidel (no email) 2005-11-28 21:30:29 PST
Comment on attachment 4601 [details]
proposed patch

This patch looks totaly sane to me.  Regardless of whether this code is
deprecated or not.
Comment 7 Darin Adler 2005-12-18 14:00:52 PST
Good point. I didn't look at the patch closely enough when I first reviewed it. Now I get it. Landing it today.