434301 - 'outline' (focus outline) on inline containing only block causes visual artifact (extra dots) {ib}

Status: NEW

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

Description

[Kai de Leeuw]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008051706 Minefield/3.0pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008051706 Minefield/3.0pre

See attached minimized testcase.

Reproducible: Always

Steps to Reproduce:
1.Load testcase
2.Press tab until link is focused
3.Behold the artifact above the link to the left
Actual Results:  
Artifact visible

Expected Results:  
Artifact not visible

Comment 1

[Kai de Leeuw]

Attachment: Testcase that demonstrates the problem describe in summary

Comment 2

[Kai de Leeuw]

Attachment: Screen shot that demonstrates the problem describe in summary

Comment 3

[Kai Liu]

Block-level elements are not allowed inside inline elements.  If you set display:block on the image, then you must do so for the anchor as well.

Comment 4

[Kai de Leeuw]

yeah, you are right... I feel ashamed I didn't get this earlier...

Comment 5

[Simon Pieters [:zcorpan]]

(In reply to comment #3)
> Block-level elements are not allowed inside inline elements.  If you set
> display:block on the image, then you must do so for the anchor as well.
> 

You're confusing HTML4 %block and %inline with CSS display:block and display:inline.

CSS does not forbid display:block inside display:inline.

Comment 6

[Kai de Leeuw]

So how should it be displayed?

Now it looks like this:

|      <------- this is in some older builds that I checked, like a tail sticking
-------         out from on top of the pic with focus outline, in later builds, 
| pic |         the dot
-------

Comment 7

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

What CSS says is that inlines that contain blocks get split into a piece before the block and a piece after the block; you're seeing the piece before the block get an outline around it.  That behavior probably isn't ideal...

See also bug 424236, bug 270191.

Comment 8

[Mats Palmgren (inactive)]

Attachment: Testcase #2

There seems to be some difference between UAs on this testcase.

Safari 3.1 and latest Webkit:
  no outline around empty inlines, except if 'background'
  is set (huh?).  (ie. 3 green,pruple boxes)

Opera 9.50b2 (Linux)
  no outline around empty inlines or blocks
  (and shows an outline around the focused block-in-inline block box)
Opera 9.27 (Linux)
  no outline around empty inlines
  (no outline around the focused block-in-inline block box)

IE8b1 (Windows XPSP3)
  outline around empty inlines and blocks, looks close
  to what we used to implemented before bug 424236.

Comment 9

[Mats Palmgren (inactive)]

Attachment: wip1

The change in 424236 also targets normal inlines, was that intentional?

Perhaps something like this would be better if we want to target the
block-in-inline problem specifically.

Comment 10

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

Note that one of the patches in bug 424236 isn't actually landed yet (I've been waiting for mozilla-central to open).

Right now the part of the inline after the block essentially disappears; it would be nice if the part before it did as well.  (Do we even construct frames for the part after when it would be empty?)

I think your approach is reasonable, though.

Comment 11

[Mats Palmgren (inactive)]

Attachment: Testcase #2e

Comment 12

[Mats Palmgren (inactive)]

Attachment: Testcase #2f

Comment 13

[Mats Palmgren (inactive)]

Attachment: Testcase #2g

Comment 14

[Mats Palmgren (inactive)]

(In reply to comment #10)
> Note that one of the patches in bug 424236 isn't actually landed yet

Yes -- it looks like it fixes the painting of the outline(s) for the
block-in-inline block part.  I was mostly concerned with the rendering of
normal empty inlines here, which I now see really is bug 426879.

> Do we even construct frames for the part after when it would be empty?

If there are no child frames for it, we don't.
But we do create it if there is a zero-width child frame.

Comment 15

[Mats Palmgren (inactive)]

Attachment: wip3

There are several issues:
1. we should not paint outline for "empty" special frames
2. the outlines we skip should not contribute to the overflow rect
3. we should include outlines we do paint in the proper overflow rect,
   i.e. the painting we intend to do in bug 424236 should contribute
   to each of those child frame's overflow rect -- not the anon block
   overflow rect. Otherwise the surrounding overflow will not be
   correct and damage rect calculations will also be incorrect.

and "empty" in 1. means the overflow rect without outline added
has zero width or height. (the difference can be seen in test #2g
where "after" should have an outline, even though GetRect() is zero)

You can see the problem with 3. in test #2f, using the pending patch
in bug 424236, the right part of the outline inside the last two
blocks are not refreshed properly (try scrolling/covering the window).
And the horizontal scrollbars should not be here (problem 2).

This patch fixes 1 and 2 (although a bit hacky since
IsEmptySpecialFrame() means two different things depending on when
the call is done)

Comment 16

[Mats Palmgren (inactive)]

Attachment: Patch rev. 5

I found the problems with the patch in bug 424236: the paint problem
is caused by an optimisation in nsDisplayOutline::OptimizeVisibility()
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsDisplayList.cpp&rev=3.44&root=/cvsroot&mark=586-587#573
and the scrollbar is caused by the overflow rect for the anon block
always includes the outline - we should only include it where the
outline around a child will overflow its frame rect.

AFAICT, this patch fixes this bug, bug 426879 and the remaining part
for bug 424236.  Sorry for bundling them together but I found that
some of the code could be shared.

Comment 17

[Mats Palmgren (inactive)]

Attachment: reftest2.diff

Reftests based on the attached testcases in this bug.

Comment 18

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

It looks like this patch incorporates the unlanded patch on bug 424236 (but not its tests).  So what I really need to review is the interdiff between the two.

(For what it's worth, I find it easier to review smaller patches, and I don't think it's a problem if one patch requires that another be applied first.  It's been pretty easy for me to do this since I've been using http://www.selenic.com/mercurial/wiki/index.cgi , although mq has its quirks and it's possible to mess things up pretty badly (including potentially losing work) until you get the hang of it.  If you try it remember to (1) use a versioned patch repository (hg init -c, and then hg qcommit frequently) and (2) set the default diff format to git diffs in ~/.hgrc by adding git = 1 in a [diff] section.)

Anyway, have to run now, so can't actually do the review now...

Comment 19

[Mats Palmgren (inactive)]

Attachment: reftest3.diff

Added the pending reftests from bug 424236, with some adjustments
to account for the fix in this bug.

Comment 20

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

Could you update the patch now that bug 424236 landed:
  http://hg.mozilla.org/mozilla-central/index.cgi/rev/2018e7c14383
?

Comment 21

[Mats Palmgren (inactive)]

Attachment: Patch rev. 6

Comment 22

[Mats Palmgren (inactive)]

Attachment: reftest4.diff

Reftests pass locally on Windows/Linux/OSX.

Comment 25

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

Comment on attachment 324130 [details] [diff] [review]
Patch rev. 6

>-  nsIFrame *frameForArea = aForFrame;
>-  do {
>-    nsIAtom *pseudoType = frameForArea->GetStyleContext()->GetPseudoType();
>-    if (pseudoType != nsCSSAnonBoxes::mozAnonymousBlock &&
>-        pseudoType != nsCSSAnonBoxes::mozAnonymousPositionedBlock)
>-      break;
>-    // If we're done, we really want it and all its later siblings.
>-    frameForArea = frameForArea->GetFirstChild(nsnull);
>-    NS_ASSERTION(frameForArea, "anonymous block with no children?");
>-  } while (frameForArea);

As far as I can tell, you don't have any code that replaces this loop.  I just added a reftest for this as 424236-11.html; presumably this patch breaks that test.

Comment 26

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

Comment on attachment 324130 [details] [diff] [review]
Patch rev. 6

review- primarily because of the previous comment.  However, I'd add:

>+nsLayoutUtils::ComputeChildOverflowAreaUnion(const nsIFrame* aFrame)
>+{
>+  nsRect overflowArea = nsRect(0, 0, 0, 0);

I'd prefer default initialization, since the concept we want here is "no rectangle" rather than the particular one you give; they just happen to be represented the same way right now.

(in class nsLayoutUtils)
>+  static PRBool IsSpecialAnonymousBlockFrame(const nsIFrame* aFrame)

I don't see any reason for this to be inline.

>diff --git a/layout/generic/nsFrame.cpp b/layout/generic/nsFrame.cpp
>+  // Skip special frames with an empty overflow rect (bug 434301).
>+  // See also ::ComputeOutlineRect().
>+  if ((GetStateBits() & NS_FRAME_IS_SPECIAL) && GetOverflowRect().IsEmpty())
>+    return NS_OK;

Shouldn't we do this only for the inlines?  I tend to think we actually don't want to skip the blocks.

>+static nsRect
>+ComputeOutlineRect(const nsIFrame* aFrame, PRBool* aAnyOutline,
>+                   const nsRect& aOverflowRect)

>+  if (nsLayoutUtils::IsSpecialAnonymousBlockFrame(aFrame)) {
>+    NS_ASSERTION(aFrame->GetFirstChild(nsnull), "anonymous block must have children");
>+    nsRect r = nsLayoutUtils::ComputeChildOverflowAreaUnion(aFrame);
>+    r.Inflate(inflateBy, inflateBy);
>+    r.UnionRect(aOverflowRect, r);
>+    return r;
>+  }

This UnionRect means we're actually returning something bigger than the outline rect.  It seems like ComputeOutlineRect should be renamed to something that describes what it really does.


Other than those issues (previous comment and this one), this looks good.

Comment 27

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

(In reply to comment #26)
> (From update of attachment 324130 [details] [diff] [review])
> review- primarily because of the previous comment.  However, I'd add:
> 
> >+nsLayoutUtils::ComputeChildOverflowAreaUnion(const nsIFrame* aFrame)
> >+{
> >+  nsRect overflowArea = nsRect(0, 0, 0, 0);
> 
> I'd prefer default initialization, since the concept we want here is "no
> rectangle" rather than the particular one you give; they just happen to be
> represented the same way right now.

Given the changes in bug 439567, feel free to ignore this comment if you want.

However, you probably want to update your unioning to IncludeEmpty, as there.

Any chance of a new patch here?

Comment 30

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

Bug 477370 has a bunch of good testcases.

Comment 31

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

Note that fixing bug 470118 would likely also fix this if we fixed the asymmetry by creating the inline on neither side.

Comment 32

[Jesse Ruderman]

(In reply to comment #31)
> Note that fixing bug 470118 would likely also fix this if we fixed the
> asymmetry by creating the inline on neither side.

The original testcase in bug 477370 had extra whitespace inside the inline but not inside the block.

Comment 34

[Zack Weinberg (:zwol)]

Why do we need the empty inline boxes on either side of the block box?  Wouldn't it be simpler and more efficient to avoid creating those in the first place, rather than having to recognize and ignore them all over layout?

Comment 39

[Boris Zbarsky [:bzbarsky]]

Why do we need the empty inline boxes on either side of the block box? Wouldn't it be simpler and more efficient to avoid creating those in the first place, rather than having to recognize and ignore them all over layout?

Just to answer this question, there are things (like borders and padding) that can make them nonempty. We generally don't want to reframe for border/padding changes.

Comment 40

[Emilio Cobos Álvarez (:emilio)]

Mats, do you recall enough context this bug to know why we can't just do something like this? (Ignore the comment which is wrong):

https://hg.mozilla.org/try/rev/12d7bec9f51467ed2f87bd3d090e2554b6532132

It of course fails the reftests from bug 424236:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6f9c51eff04ed349452aa577ce2d11418284566&selectedJob=239725909

But looking at the analyzer we could probably just tweak the expectation.

Comment 41

[Mats Palmgren (inactive)]

I seem to recall that I fixed the calculation of the actual overflow rects
rather than just suppressing painting the outlines in some cases.
But yeah, we could take your patch as a wallpaper since I think it
probably covers the common cases. You should change GetRect() to
GetScrollableOverflowRect() though, and do the same for
mozBlockInsideInlineWrapper (see below). (Probably better to file
a separate bug for that improvement since I think we still might
want to fix some of the overflow calculations here.)

(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 from comment #26)

Shouldn't we do this only for the inlines? I tend to think we actually
don't want to skip the blocks.

I'm pretty sure I also skipped empty blocks intentionally.
I can't think of a good reason to include them.
Compare this in Chrome:

<span style="outline: 5px solid red">
  A
  <div style="height:0"></div>
  B
</span>

Comment 43

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 44

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 10 duplicates.
:jfkthame, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 45

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.