276236 - Contents w/ opacity in <button> laid out incorrectly

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[HARUNAGA Hirotoshi]

Contents with opacity < 1 inside <button> are laid out incorrectly
as if other floater didn't exist.

Steps:
Load the URL.

Actual result:
Contents with opacity < 1 inside <button> are laid out too left.

Another testcase in which <button>s are not in floater:
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2522&action=view
This causes similar problem, although the result depends on reloading,
back/forward and Site Navigation Bar enabled/disabled.

Comment 1

[Mats Palmgren (inactive)]

Similar to bug 131698 - after re-parenting a frame we need to update
NS_FRAME_HAS_CHILD_WITH_VIEW on the new ancestors.

Comment 2

[Mats Palmgren (inactive)]

Attachment: Testcase

Comment 3

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

If this is always just moving frames constructed for the parent to the child,
then why not just propagate the ...HAS_CHILD_WITH_VIEW bit from the parent to
the child?  If not, then shouldn't you be making sure to reparent the views (as
the helper functions in nsHTMLContainerFrame do)?

Comment 5

[Mats Palmgren (inactive)]

(In reply to comment #4)
> If this is always just moving frames constructed for the parent to the child

ReParentFrameList() is called from SetInitialChildList(), AppendFrames(),
InsertFrames() and ReplaceFrame() - I wasn't sure I could trust the
parent pointers for the given list of frames to have any specific value...

After some tests it looks like it's always set to the ButtonControlFrame (this)
or its area frame (mFrames.FirstChild()). However, unless someone can say for
sure that this is always true, I will extend the current patch to do view
re-parenting also...

Comment 6

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Reparent views also.
(I couldn't find a testcase where this was actually needed though).

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Well, it looks like it would be, since those methods should normally be called
when changing the child list of the frame itself.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> it looks like it's always set to the ButtonControlFrame (this)
> or its area frame (mFrames.FirstChild()). 

That's correct.  If it's not one of those two, someone screwed up badly.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Which means this should me much simpler -- just propagate the
NS_FRAME_HAS_CHILD_WITH_VIEW bit from parent to child.

Comment 10

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 169862 [details] [diff] [review]
Patch rev. 3

We never remove NS_FRAME_HAS_CHILD_WITH_VIEW anywhere else, so

mFrames.FirstChild()->AddStatebits(GetStateBits() &
NS_FRAME_HAS_CHILD_WITH_VIEW);

should be sufficient.

Comment 12

[Mats Palmgren (inactive)]

Attachment: Patch rev. 4

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 169872 [details] [diff] [review]
Patch rev. 4

r=bzbarsky

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 169872 [details] [diff] [review]
Patch rev. 4

What I wrote in comment 11 is probably considerably more efficient (esp. on
heavily pipelined processors) and easier for compilers to optimize to the other
form in case it's not, but sr=dbaron.

Comment 15

[Mats Palmgren (inactive)]

Attachment: Patch rev. 5

Sorry, I did not pay attention so I missed that you suggested an alternative.
Of course we should check in the most effecient version...
A quick r+ anyone?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 170069 [details] [diff] [review]
Patch rev. 5

r=bzbarsky

Comment 17

[Mats Palmgren (inactive)]

Checked in 2005-01-02 23:27 PDT.

The leak stats on 'balsa' changed slightly, I find it hard to believe this
patch can have anything to do with it, but I'll keep an eye open.
Let's see what 'brad' says next...

-> FIXED

Comment 18

[Mats Palmgren (inactive)]

'balsa' went back to the original numbers on the next cycle,
'brad' did not show any difference, so there was no problem.