283459 - [residual style]text color is not as expected

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Devoti Paolo]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050222
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050222

At the provided url text is displayed with default color instead of the
specified one.


Reproducible: Always

Steps to Reproduce:
1.open url
Actual Results:  
overlapped text has same color

Expected Results:  
overlapped text has different color

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

(Hmm, that page has a lot of markup errors, and some CSS errors... oh well)

I'm unsure what exactly you mean... I think the basic issue is this:

  <div class="foo">
    <a href="bar">asdf</a>
  </div>

With a style like:
  <style>.foo { color:silver; }</style>

And you expect the link to be silver, right?

If so, I think this bug is invalid... the pref stylesheet matches the a itself,
and that overwrites inherited styles.

Comment 2

[Devoti Paolo]

Just to tell that I'm not involved at all in the writing of this page.
I'm only providing a "works with other browser" fact and can't tell on my own if
is a bug or a bogous page rendered as possible.  

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: behaves somewhat better

This patch makes us inconsistently do the "correct" thing on this testcase. By
"inconsistently", I mean that if we receive the document in chunks, then if a
chunk falls between one <a> and its </a>, then we do the "wrong" thing.
Otherwise, we'll do what's expected. I'm not sure if this is an improvement or
not .

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 175513 [details] [diff] [review]
behaves somewhat better

r=bzbarsky.

jst, can you sr?

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 175513 [details] [diff] [review]
behaves somewhat better

Could someone explain what this does in more detail? Making us work
inconsistently doesn't seem like a cool thing to do, even if on average we'd be
more "correct", whatever that means here...

Comment 6

[Devoti Paolo]

Attachment: rendered as autor intended it

Displayed text obeys to "top" and "left" but not to "color"

Comment 7

[Devoti Paolo]

Attachment: testcase

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #5)
> Could someone explain what this does in more detail?

In order to help with performance, we occasionally let inlines contain blocks.
This happens when we are sure that the inline full contains the blocks (this is
called being well formed). So in the case of
<inline><block>...</block></inline>, the content model looks exactly like the
source.

In order to accomplish this, whenever we finish tokenizing a chunk of input, we
call nsHTMLTokenizer::ScanDocStructure(). This function looks at all of the
available tokens and indicates (through SetContainerInfo) whether an inline
should be able to contain any children. If the chunk is incomplete (i.e., if
when we tokenize, we only see "<inline><block>", then we call the inline not
wellformed, and perform RS handling on it (closing it before the block and
reopening it in the block)). A while ago harishd added the idea of "flushing"
tokens after certain tags (read: <script> and <style> tags). See bug 22485. When
we flush tags, ScanDocStructure doesn't see any close tags after the flushing
tag, so it doesn't mark any of the tags as well formed.

In this case, the <style> tag was (incorrectly) causing flushing, so the <a>s
were all considered not-well formed. With my patch, the <a>s *can* be marked
well formed, but if a packet boundary occurs between one <a> and its </a>, then
we don't mark it as well formed, thus we see what looks like inconsistent
behavior. Also note: even if the <style> is moved out of <a>s, we face the same
packet boundary problem, which would also mean inconsistent behavior.

Comment 9

[Johnny Stenback (:jst)]

Yeah, that's what I'm not feeling too good about. I don't like introducing such
an inconsistency, that'd lead to even more bugs on odd behaviour. I'd rather us
be "broken" than having seemingly random results from parsing a page. Parse it
again and the content may come out of the cache in different sized chunks and we
get different behaviour than we originally got. Not cool, IMO.

Or am I missing something obvious here?

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 175513 [details] [diff] [review]
behaves somewhat better

So I talked this over with Blake online, and I'm still not convinced. I really
don't like introducing the inconsistent behaviour here, if we could fix that
somehow I'd be fine with this change, but w/o such a fix I'm going to minus
this, at least until someone convinces me that the gain is worth the
inconsistent behaviour...

Comment 11

[Boris Zbarsky [:bzbarsky]]

jst, the problem is that right now we have inconsistent behavior for all blocks
that don't contain <style> or <script>.  For those blocks that _do_ contain
<style> or <script> we consistently have the wrong behavior....

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 175513 [details] [diff] [review]
behaves somewhat better

Alright, I'm still not getting a warm fuzzy feeling here, but sr=jst :)

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in.