Closed Bug 541078 Opened 15 years ago Closed 15 years ago

[HTML5] contentEditable reload: docshell/test/navigation/test_bug430624.html fails

Categories

(Core :: DOM: HTML Parser, defect, P1)

Product:
Core
Component:
DOM: HTML Parser
Platform:
x86
macOS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: hsivonen, Assigned: hsivonen)

References

Details

Attachments

(1 file, 1 obsolete file)

Make BeginLoad ensure that editing is off
2.58 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
With the HTML5 parser enabled docshell/test/navigation/test_bug430624.html fails. The data: URL document reloads back to its initial state instead of navigating back to its edited state. This might have something to do with bug 541050, too.
Summary: [HTML5] docshell/test/navigation/test_bug430624.html fails → [HTML5] contentEditable reload: docshell/test/navigation/test_bug430624.html fails
It looks like the editor required the parser to break out of a doc update batch after inserting the root element node.
Hmm. Even though the old parser turns off editing on the document upon <html> before turning it back on upon <body>, making the HTML5 parser break out of the doc update on <html> doesn't make this test case pass. However, breaking on nsHTMLDocument::EditingStateChanged() in a debugger and continuing after a moment makes the test pass... Odd.
It appears that when the editor is supposed to reattach itself, the flags on the document say it already has an editor. What are the expectations of the mechanism that detaches the editor?
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Attachment #431332 - Flags: review?(ehsan.akhgari)
Summary: [HTML5] contentEditable reload: docshell/test/navigation/test_bug430624.html fails → [HTML5][Patch] contentEditable reload: docshell/test/navigation/test_bug430624.html fails
That seems really wrong, the parser shouldn't deal with editing stuff. (In reply to comment #3) > It appears that when the editor is supposed to reattach itself, the flags on > the document say it already has an editor. I think we don't reset the state on page hide, but why is this a problem? Reattaching doesn't check that afaik.
Comment on attachment 431332 [details] [diff] [review] Make sure editing gets turned off at the start of the parse the same way it's turned off by document.open() I think peterv is right here, this is probably not something to fix this way. Do we have any idea what is the actual reason of the test failure? It seems to me like an underlying issue which this patch puts a band-aid on.
Attachment #431332 - Flags: review?(ehsan.akhgari) → review-
(In reply to comment #5) > That seems really wrong, the parser shouldn't deal with editing stuff. > > (In reply to comment #3) > > It appears that when the editor is supposed to reattach itself, the flags on > > the document say it already has an editor. > > I think we don't reset the state on page hide, but why is this a problem? With the old parser, when <html> is inserted and the doc update batch immediately ended, editing gets turned off as a side effect of the update batch ending. Then when <body> is inserted and the update batch immediately ended, editing gets turned back on. Now, with the HTML5 parser, the doc update batch doesn't end when <html> has been inserted, so editing doesn't get turned off near the start of the parse. (I also tried making the doc update batch end right there, but it didn't help. I don't know why.) When <body> is inserted, the HTML5 parser breaks out of the doc update batch as of the patch for bug 540574. At this point, the document doesn't actually have an editor, but it's editing state claims that it has. Therefore, the old and new editing state are the same, so the document doesn't get an editor. On the general level, the underlying issue seems to be that the editing state logic relying on doc update batches ending early and often even though (as I understand it), the doc update batches are supposed to be allowed to be arbitrarily long. The HTML5 parser tries to make the batches as long as possible.
Would it be acceptable to make nsHTMLDocument::BeginLoad ensure that editing is off?
Summary: [HTML5][Patch] contentEditable reload: docshell/test/navigation/test_bug430624.html fails → [HTML5] contentEditable reload: docshell/test/navigation/test_bug430624.html fails
Could you try to call SetEditingState(eOff) in nsDocShellEditorData::DetachFromWindow, just after we called GetEditingState?
(In reply to comment #9) > Could you try to call SetEditingState(eOff) in > nsDocShellEditorData::DetachFromWindow, just after we called GetEditingState? I tried it. The test case failed still.
(In reply to comment #7) > With the old parser, when <html> is inserted and the doc update batch > immediately ended, editing gets turned off as a side effect of the update batch > ending. Then when <body> is inserted and the update batch immediately ended, > editing gets turned back on. > > Now, with the HTML5 parser, the doc update batch doesn't end when <html> has > been inserted, so editing doesn't get turned off near the start of the parse. > (I also tried making the doc update batch end right there, but it didn't help. > I don't know why.) Are we sure that changing the behavior of update batches doesn't have any other side effects? I guess if all else was equal, making sure that the HTML5 parser has the same behavior as the old parser as far as update batches were concerned should have solved this issue. It appears to me that there is another underlying issue present here which we don't know about yet.
(In reply to comment #11) > (In reply to comment #7) > > With the old parser, when <html> is inserted and the doc update batch > > immediately ended, editing gets turned off as a side effect of the update batch > > ending. Then when <body> is inserted and the update batch immediately ended, > > editing gets turned back on. > > > > Now, with the HTML5 parser, the doc update batch doesn't end when <html> has > > been inserted, so editing doesn't get turned off near the start of the parse. > > (I also tried making the doc update batch end right there, but it didn't help. > > I don't know why.) > > Are we sure that changing the behavior of update batches doesn't have any other > side effects? No we aren't *sure*, but so far, the only other *known* case was that style sheet linking elements posted a useless script runner that needed to be extinguished early in the parser-inserted case. Going forward, that case could relatively easily be addressed by making style sheet linking element factory methods observe the "parser-inserted flag" available to element factories and not post the script runner at all the parser-inserted case. > I guess if all else was equal, making sure that the HTML5 parser > has the same behavior as the old parser as far as update batches were concerned > should have solved this issue. I don't recall the exact details of the discussions that lead to the current design of making the doc update batches as long as possible, but the motivation of making them long are three-fold: 1) Supposed performance benefit. (Never quantified.) 2) Preventing script-like things from running when the parser is building the tree (except in the well-defined points meant for running actual <scripts>). (This is no longer quite true, because the <body>, <link>, </style> and SVG </style> now break out of the update and reopen it right away after checking if ending the update had the size effect of something calling nsIParser::Terminate().) 3) Never let any scripts or script runners or anything of that nature to see the DOM in a state where nodes in the DOMs haven't been notified on. Of these points, at least #3 was very a deliberate change from the old parser and was discussed in content team meetings. Given the goal of getting the HTML5 enabled this quarter, I view the prospects of doing something that'd change point #3 very negatively and would favor applying band-aid of the nature of the r-'ed patch instead (possibly moved into nsHTMLDocument::BeginLoad() to avoid exposing a new public method for the parser to call directly). If the HTML5 parser broke out of the doc update batch after each element insertion, it would either have to notify on per-insertion basis (something I've been explicitly instructed to avoid) or to make it possible for script runners to run against nodes that haven't been notified on yet (something we've decided to avoid as an improvement over the old parser). The documented API contract for BeginUpdate/EndUpdate at http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIDocument.h only talks vaguely about a batch of modifications without defining how long the batches are supposed to be. Has update batching always had an unwritten API contract that limits the length of the update to at most one element insertion per batch? Even there were such an unwritten aspect to the API contract, if we want the future to be that update batches can be longer, turning editing off at the start of the parser explicitly would be a less invasive transitional solution than changing how the HTML5 parser does update batching.
> Has update batching always had an unwritten API contract that limits the length > of the update to at most one element insertion per batch? Not that I know of. In fact, we have code that does multiple inserts per batch right now (insertNode on a document fragment). Fwiw, point #3 in comment 12 is very important. We really want that. Once lazy frame construction lands, notifying more often _might_ be ok. Maybe. Would need to be very careful with it.
Comment on attachment 431332 [details] [diff] [review] Make sure editing gets turned off at the start of the parse the same way it's turned off by document.open() (In reply to comment #13) > > Has update batching always had an unwritten API contract that limits the length > > of the update to at most one element insertion per batch? > > Not that I know of. In fact, we have code that does multiple inserts per batch > right now (insertNode on a document fragment). > > Fwiw, point #3 in comment 12 is very important. We really want that. > > Once lazy frame construction lands, notifying more often _might_ be ok. Maybe. > Would need to be very careful with it. Let's see what Boris thinks about this patch. If he's fine with it, I have no objections. :-)
Attachment #431332 - Flags: review?(bzbarsky)
This patch doesn't introduce new public methods and handles the HTML5 parser's needs and the document.open() needs in one place.
Attachment #431332 - Attachment is obsolete: true
Attachment #432132 - Flags: review?(bzbarsky)
Attachment #431332 - Flags: review?(bzbarsky)
Comment on attachment 432132 [details] [diff] [review] Make BeginLoad ensure that editing is off This seems no worse than the rest of the editor attach/detach mess...
Attachment #432132 - Flags: review?(bzbarsky) → review+
Thank you. http://hg.mozilla.org/mozilla-central/rev/51d36e8f461d
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: