131698 - iframe (and some form elements) & CSS 'p:first-line' don't play nice together

Status: RESOLVED WORKSFORME

(Core :: Layout: Block and Inline, defect)

Description

[GavinS]

If I have an IFRAME near the first line of a paragraph, then seemingly any CSS
rule containing 'p:first-line' will cause the IFRAME to be placed in the top
left of the window (exactly in the top left, overwriting anything else that
should be there).

I have tried with many combinations of number of iframes, how they are
positioned, and css attributes for the p:first-line style.

If I have more than 1 iframe, they are all placed on top of each other in the
top left of the window.

If the iframe is just at the start of a <p>, or after a small amount of text in
a <p>, or if the <p> and the iframe are within table cells, the iframe is still
displayed in the top left of the window.

I have tried margin, border, font-weight and text-decoration as the styles to
apply to the 'p:first-line', and in all cases the iframe is displayed in the top
left of the window.

Note that I'm not a CSS or html expert at all, but I *think* what I'm trying to
do is valid :)

Comment 1

[GavinS]

Attachment: test case: the iframe overwrites the text in the top left of the window

This test case validates at w3.

Comment 2

[Arthur]

Confirming on Linux 2002031721. Weird...

Comment 3

[Arthur]

Attachment: Testcase without the first-line rule to show the difference.

Comment 4

[Arthur]

It helps if you put a closing </p> before the iframe-tag. But that doesn't
explain the difference between the two testcases.

Comment 5

[Mats Palmgren (inactive)]

Attachment: Testcase #3

It seems like the :first-line selector makes the iframe reset its position to
0,0

Comment 6

[Christine Hoffman]

Changing QA contact

Comment 7

[GavinS]

2 extra things:

a) Mozilla 0.7 did not show this problem, and 

b) I think that combobox/select boxes also suffer from the same problem as
iframes: i.e. they get repositioned to (0,0) if they are on a line with a
p::first-line css setting

Comment 8

[Boris Zbarsky [:bzbarsky]]

.

Comment 9

[Mats Palmgren (inactive)]

*** Bug 209489 has been marked as a duplicate of this bug. ***

Comment 10

[Mats Palmgren (inactive)]

Attachment: Testcase #4

Comment 11

[Mats Palmgren (inactive)]

The reason this bug occurs is this optimization:

void
nsContainerFrame::PositionChildViews(nsIPresContext* aPresContext,
                                     nsIFrame*       aFrame)
{
  nsFrameState frameState;
  aFrame->GetFrameState(&frameState);
  if (! ((frameState & NS_FRAME_HAS_CHILD_WITH_VIEW) ==
              NS_FRAME_HAS_CHILD_WITH_VIEW)) {
    return;
  }

  ... position the views ...


The problem is that the NS_FRAME_HAS_CHILD_WITH_VIEW bit has not been setup
correctly on the nsFirstLineFrame so we will not reach the code that do
the view positioning.  I have a patch that seems to fix the problem...

Comment 12

[Mats Palmgren (inactive)]

Attachment: Patch

Comment 13

[Mats Palmgren (inactive)]

There is still some flicker when the IFRAME is not on the first line (load
Testcase #4 and make the window narrow) but I believe that is a separate bug.

Comment 14

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

Could you use the new functions (|GetStateBits| and |AddStateBits|) rather than
the old ones?  Also, you need to check NS_FRAME_HAS_VIEW on the child as well. 
Finally, the loop would probably be better written as a while loop, and without
modifying the parameter (which is generally unclear), i.e., with a new variable
(perhaps just |f|).

(|ReparentFrame| and |MoveChildrenTo| are looking more and more similar.  But
probably it's still better to keep them separate.)

Comment 15

[Mats Palmgren (inactive)]

Will do, but regarding NS_FRAME_HAS_VIEW on the child - isn't it possible that
the child does not have a view but have a grandchild that has one and thus
it will have NS_FRAME_HAS_CHILD_WITH_VIEW without being the one with the view?
or does NS_FRAME_HAS_VIEW propagate upward too?

Comment 16

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

I think you need to check if *either* is set -- NS_FRAME_HAS_CHILD_WITH_VIEW
really means descendant, but I don't think it includes self, so I think (maybe
I'm wrong, though) that a frame with a view and no descendants with views has
NS_FRAME_HAS_VIEW but not NS_FRAME_HAS_CHILD_WITH_VIEW.

Comment 17

[Mats Palmgren (inactive)]

OK, got it, thanks.

There is also the question if one should propagate NS_FRAME_HAS_CHILD_WITH_VIEW
to a frame that has a view of its own. Looking at nsIFrame::SetView() I think so.
That would cover the case the view goes away for some reason.
In other words, we should look for either flag on the child but only 
NS_FRAME_HAS_CHILD_WITH_VIEW on the parents.

BTW, shouldn't MoveChildrenTo() propagate the flag to all the parents?

Comment 18

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Comment 19

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

Comment on attachment 127616 [details] [diff] [review]
Patch rev. 2

>@@ -11875,6 +11875,21 @@
> {
>   aPresContext->ReParentStyleContext(aFrame, aParentStyleContext);
>   aFrame->SetParent(aNewParentFrame);
>+
>+  // Set NS_FRAME_HAS_CHILD_WITH_VIEW on the parents if needed, bug 131698.

They're ancestors, not parents.  (Yes, parent and child are currently used in
the code a lot where ancestors and descendants are meant.  That doesn't mean we
should continue to do it.)

>+  if (!(aNewParentFrame->GetStateBits() & NS_FRAME_HAS_CHILD_WITH_VIEW) &&
>+      (aFrame->GetStateBits() &
>+       (NS_FRAME_HAS_VIEW | NS_FRAME_HAS_CHILD_WITH_VIEW))) {
>+    // aFrame or one if its descendants has a view.
>+    aNewParentFrame->AddStateBits(NS_FRAME_HAS_CHILD_WITH_VIEW);
>+
>+    // Propagate to all parents, we are done if we find one with the bit set.
>+    nsIFrame* parent = aNewParentFrame->GetParent();

You're just pulling the first iteration of the loop out of the loop here, which
just means extra code.	I'd think you could simplify the whole thing to what's
below.

>+    while (parent && !(parent->GetStateBits() & NS_FRAME_HAS_CHILD_WITH_VIEW)) {
>+      parent->AddStateBits(NS_FRAME_HAS_CHILD_WITH_VIEW);
>+      parent = parent->GetParent();
>+    }
>+  }

The simplification (I think) would be:

  if (aFrame->GetStateBits &
      (NS_FRAME_HAS_VIEW | NS_FRAME_HAS_CHILD_WITH_VIEW))
    for (nsIFrame *f = aNewParentFrame;
	 f && !(f->GetStateBits() & NS_FRAME_HAS_CHILD_WITH_VIEW);
	 f = f->GetParent())
      f->AddStateBits(NS_FRAME_HAS_CHILD_WITH_VIEW);

> There is also the question if one should propagate NS_FRAME_HAS_CHILD_WITH_VIEW
> to a frame that has a view of its own. Looking at nsIFrame::SetView() I think so.
> That would cover the case the view goes away for some reason.
> In other words, we should look for either flag on the child but only 
> NS_FRAME_HAS_CHILD_WITH_VIEW on the parents.

NS_FRAME_HAS_CHILD_WITH_VIEW is an optimization used by
nsContainerFrame::PositionChildViews.

> BTW, shouldn't MoveChildrenTo() propagate the flag to all the parents?

MoveChildrenTo *may* operate on the assumption that it's moving the children
between siblings.  I might be wrong, though.  And whatever it is should
probably be documented sometime...

Comment 20

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

> NS_FRAME_HAS_CHILD_WITH_VIEW is an optimization used by
> nsContainerFrame::PositionChildViews.

I meant to add that yes, it should be set.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Hmm... the testcases in this bug seem to work for me...

Comment 22

[Anne (:annevk)]

WFM: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6b) Gecko/20031119

Comment 23

[Asa Dotzler [:asa]]

This bug worksforme with Windows Seamonkey builds 2004020909