Closed
Bug 283861
Opened 19 years ago
Closed 16 years ago
Want Text3.isElementContentWhitespace
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla1.9.1b2
People
(Reporter: caillon, Assigned: WeirdAl)
References
()
Details
Attachments
(2 files, 2 obsolete files)
701 bytes,
patch
|
sicking
:
review+
sicking
:
superreview+
|
Details | Diff | Splinter Review |
1.01 KB,
text/html
|
Details |
To make it easier to figure out what content is ignorable from apps.
Reporter | ||
Comment 1•19 years ago
|
||
Reporter | ||
Updated•19 years ago
|
Attachment #175668 -
Flags: superreview?(jst)
Attachment #175668 -
Flags: review?(jst)
Comment 2•19 years ago
|
||
Does this really not need to be done on character data nodes, etc? Also, why not implement getWholeText/replaceWholeText? Those shouldn't be too hard, should they?
Comment 3•19 years ago
|
||
Comment on attachment 175668 [details] [diff] [review] nsIDOM3Text r+sr=jst
Attachment #175668 -
Flags: superreview?(jst)
Attachment #175668 -
Flags: superreview+
Attachment #175668 -
Flags: review?(jst)
Attachment #175668 -
Flags: review+
Assignee | ||
Comment 4•18 years ago
|
||
*** Bug 343906 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 5•17 years ago
|
||
Comment on attachment 175668 [details] [diff] [review] nsIDOM3Text This is entirely new code, which has somehow sat around, not checked in, for a very long time...
Attachment #175668 -
Flags: approval1.9?
Comment 6•17 years ago
|
||
That patch looks bitrotted to me. For one thing, the tearoff needs to implement cycle collection.
Comment 7•17 years ago
|
||
And shipping something that will start throwing exceptions on enumeration at this stage in the game worries me in any case.
Comment 8•17 years ago
|
||
Comment on attachment 175668 [details] [diff] [review] nsIDOM3Text I agree with Boris here. I don't want to approve this until we know such an old patch is still valid. Please test and re-request approval when done.
Attachment #175668 -
Flags: approval1.9? → approval1.9-
Assignee | ||
Comment 9•16 years ago
|
||
Reading current code in mozilla-central against the patch, it looks like most of the code is already in place - the one major difference being nsITextContent doesn't exist anymore, and various methods are using nsIContent's TextIsOnlyWhitespace(). Given that, I am crafting an updated patch for this bug, but I would love it if someone would write a mochitest for it.
Assignee | ||
Comment 10•16 years ago
|
||
Assignee: caillon → ajvincent
Attachment #175668 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #341580 -
Flags: superreview?(jonas)
Attachment #341580 -
Flags: review?(jonas)
Attachment #341580 -
Flags: superreview?(jonas)
Attachment #341580 -
Flags: superreview+
Attachment #341580 -
Flags: review?(jonas)
Attachment #341580 -
Flags: review+
Assignee | ||
Comment 11•16 years ago
|
||
Whoops, I almost repeated caillon's mistake of not looking to get this checked in. ;-)
Keywords: checkin-needed
Comment 12•16 years ago
|
||
Would be great to get some tests here - to prevent regressions.
Assignee | ||
Updated•16 years ago
|
Flags: in-testsuite?
Comment 13•16 years ago
|
||
Not quits sure if this is what you guys were looking for and I don't have a build system setup to test this stuff out. Feel free to modify as you see fit.
Assignee | ||
Comment 14•16 years ago
|
||
Ryan, the code returns true if the text node's whole contents are white space.
Comment 15•16 years ago
|
||
(In reply to comment #14) > Ryan, the code returns true if the text node's whole contents are white space. That should be simpler to test for then. I'll re-write the tests.
Comment 16•16 years ago
|
||
This should be better then. Would appreciate it is someone could test and see if it actually does what it should though!
Attachment #343251 -
Attachment is obsolete: true
Comment 17•16 years ago
|
||
Comment on attachment 341580 [details] [diff] [review] updated patch [Checkin: Comment 17] http://hg.mozilla.org/mozilla-central/rev/4755421bae8f
Attachment #341580 -
Attachment description: updated patch → updated patch
[Checkin: Comment 17]
Updated•16 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
OS: Linux → All
QA Contact: ian → general
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
Comment 18•16 years ago
|
||
Our DOM doesn't have a way to distinguish ignorable whitespace from regular whitespace. I'd rather we didn't implement this instead of implementing it differently from what the specification defines.
Assignee | ||
Comment 19•16 years ago
|
||
peterv: do you mean we should back this patch out?
Assignee | ||
Comment 20•14 years ago
|
||
I've been thinking about this lately. I think the gist of comment 18 is that because we're not a validating XML processor, there's no definition of ignorable white space as far as we're concerned: http://www.w3.org/TR/REC-xml/#sec-white-space I don't see any way to improve on what we've got, even with regular expressions work being done for bug 345512.
Assignee | ||
Updated•6 years ago
|
Flags: in-testsuite?
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•