Summary: | Implement textContent property | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Anders Carlsson <andersca> | ||||||
Component: | DOM | Assignee: | Anders Carlsson <andersca> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | cdumez | ||||||
Priority: | P2 | ||||||||
Version: | 420+ | ||||||||
Hardware: | Mac | ||||||||
OS: | OS X 10.4 | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 3249 | ||||||||
Attachments: |
|
Description
Anders Carlsson
2005-10-31 10:44:15 PST
Created attachment 4542 [details]
Implement textContent
Looks great! I think i'd like a secondary approval from hyatt, darin or mjs though. Comment on attachment 4542 [details]
Implement textContent
The check of value->isNull() in TextContent makes me think two things:
1) I'd like a helper routine that creates a DOMString that handles the
JavaScript null case in this way. I'm sure there are other functions like this
one elsewhere in the DOM.
2) What about undefined? Should undefined set the text contents to "undefined"?
If not, then it should be isUndefinedOrNull() rather than isNull().
I think the code in NodeImpl::textContent and setTextContent cries out for
either a switch statement or a virtual function.
The code here is not the same as in setInnerText, but it should be. If they
shared code, then we would have the empty text check in both places
(setInnerText currently lacks it).
The check of text.isNull() should be a check of text.isEmpty() I believe
(unless I'm missing something).
(In reply to comment #3) > (From update of attachment 4542 [details] [edit]) > The check of value->isNull() in TextContent makes me think two things: > > 1) I'd like a helper routine that creates a DOMString that handles the > JavaScript null case in this way. I'm sure there are other functions like this > one elsewhere in the DOM. I can't find any other functions like this so I think I'll just let it be. > > 2) What about undefined? Should undefined set the text contents to "undefined"? > If not, then it should be isUndefinedOrNull() rather than isNull(). > I think it makes more sense to set the text to "undefined". This is what Mozilla does. > I think the code in NodeImpl::textContent and setTextContent cries out for > either a switch statement or a virtual function. > Yeah, I've changed it to do that. > The code here is not the same as in setInnerText, but it should be. If they > shared code, then we would have the empty text check in both places > (setInnerText currently lacks it). > They should not be the same. setInnerText should ideally do parsing (converting \n to <br> for example) but setTextContent shouldn't do that so I'd rather have them separate functions. > The check of text.isNull() should be a check of text.isEmpty() I believe > (unless I'm missing something). > Yeah, that's correct. I've changed it. Created attachment 4547 [details]
Address comments
Comment on attachment 4547 [details]
Address comments
r=me
Mass moving XML DOM bugs to the "DOM" Component. |