236175 - If inline element has padding that value is <percentage>, its text-decoration is broken.

Status: RESOLVED FIXED

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

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

When inline element has {padding: n%;},
its text-decoration is broken.

this bug comes bugzilla-jp.
http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=3655

Reproducible: Always
Steps to Reproduce:
1.See next test cases.
test case 1 http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2050&action=view
test case 2 http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2049&action=view

Actual Results:  
In test case 1, using <percentage> case is broken.
And in test case 2, underline and overline is lost.
and line-through position is worong.

Expected Results:  
Underline and overline is rendered in all cases.
And line-through is positioned correctly in all cases.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=3655#c19

> nsHTMLContainerFrame::PaintTextDecorations
> nsHTMLContainerFrame::PaintTextDecorationLines
> {
>   nsMargin bp;
>   CalcBorderPadding(bp);
> 　nscoord innerWidth = mRect.width - bp.left - bp.right;
>   aRenderingContext.FillRect(bp.left, 
>                              bp.top + aAscent - aOffset, innerWidth, aSize);
> }
> 
> NS_IMETHODIMP  nsFrame::CalcBorderPadding(nsMargin& aBorderPadding) const {
>   NS_ASSERTION(mStyleContext!=nsnull,"null style context");
>   if (mStyleContext) {
>     nsStyleBorderPadding bpad;
>     mStyleContext->GetBorderPaddingFor(bpad);
>     bpad.GetBorderPadding(aBorderPadding);　　　　　# fail here!(when using %)
>     return NS_OK;
>   }
>   return NS_ERROR_FAILURE;
> }

When using percentage, bpad.GetBorderPadding(aBorderPadding) is faild.
So, "bp" is indefinite.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Yeah, this will at least give us consistent (if incorrect) behavior....

Comment 3

[Hideo Saito]

Unless each value of top, left, right, and bottom of aBorderPadding has an
effective value, the length or the position of a line are not displayed correctly.

Comment 4

[Hideo Saito]

Attachment: patch2

Can this patch be evaluated?

-    bpad.GetBorderPadding(aBorderPadding);
+    if (!bpad.GetBorderPadding(aBorderPadding)) {
+      const nsStylePadding* paddingStyle = GetStylePadding();
+      paddingStyle->CalcPaddingFor(this, aBorderPadding);
+    }

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 142887 [details] [diff] [review]
patch2

Yeah, this is probably better (though CalcPaddingFor is rather evil, sometimes
buggy, and deprecated, as the comments say...)

We should really consider storing computed padding on frames... maybe only do
it if it's percent-based and store it as a property?

Comment 6

[Hideo Saito]

In the function of CalcSideFor in shared/src/nsStyleStruct.cpp, the influencing
code is considered to be only the case of nsStyleUnit is eStyleUnit_Percent.

Comment 7

[Hideo Saito]

shared/src/nsStyleStruct.cpp -> content/shared/src/nsStyleStruct.cpp

Comment 8

[Boris Zbarsky [:bzbarsky]]

Hideo Sato, what are you actually trying to say in comment 6?

Comment 9

[Hideo Saito]

The patch means that only the following code was related.

the function of CalcSideFor at content/shared/src/nsStyleStruct.cpp
  nscoord result = 0;
    case eStyleUnit_Percent:
      {
        nscoord baseWidth = 0;
        PRBool  isBase = PR_FALSE;
        nsIFrame* frame = aFrame->GetParent();
        while (frame) {
          frame->IsPercentageBase(isBase);
          if (isBase) {
            baseWidth = frame->GetSize().width;
            // subtract border of containing block
            nsMargin border;
            frame->GetStyleBorder()->CalcBorderFor(frame, border);
            baseWidth -= (border.left + border.right);
            // if aFrame is not absolutely positioned, subtract
            // padding of containing block
            const nsStyleDisplay* displayData = aFrame->GetStyleDisplay();
            if (displayData->mPosition != NS_STYLE_POSITION_ABSOLUTE &&
                displayData->mPosition != NS_STYLE_POSITION_FIXED) {
              nsMargin padding;
              frame->GetStylePadding()->CalcPaddingFor(frame, padding);
              baseWidth -= (padding.left + padding.right);
            }
            break;
          }
          frame = frame->GetParent();
        }
        result = (nscoord)((float)baseWidth * aCoord.GetPercentValue());
      }
      break;

Comment 10

[Boris Zbarsky [:bzbarsky]]

Yes, of course.  Thing is, that whole function is deprecated (as the comments
say) and we'd sorta like to get rid of it rather than adding new callers...

Comment 11

[Hideo Saito]

I understood that the patch has a problem. For example, when the image has been
arranged on left side, the length or the position of a line are not displayed
correctly. It is likely to be necessary to consider another method.

Comment 12

[Hideo Saito]

Although I tested using mozilla-1.6, there was no errors by using mozilla-1.7a.
A value of aContainingBlockWidth of the function
nsHTMLReflowState::InitConstraints is differ in mozilla-1.6 and mozilla-1.7a.
It seems that the value of mozilla-1.6 is wrong.

Comment 13

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

Changed due to bug 97695?

Comment 14

[Hideo Saito]

Yes, that was just changed.

Comment 15

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

This patch isn't going to work when borders are present.

We really need to clean up all these border/padding/margin calculation
functions.  We've got a bunch of them, but they're really all broken -- we
should have a simpler way of getting the right answer than creating an
nsHTMLReflowState -- and we should use the same code everywhere (i.e., we should
have little structs that do the calculation -- and nsHTMLReflowState should use
them just like everyone else).

Comment 16

[Hideo Saito]

Attachment: testcase for border-width

Comment 17

[Hideo Saito]

Attachment: patch3

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 143403 [details] [diff] [review]
patch3

David is right... we should spend some time and make this whole setup sane.

Comment 19

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

Comment on attachment 143403 [details] [diff] [review]
patch3

Of course, this still doesn't work for percentage borders, and it also gives
the wrong results for percentage padding, and if you made it work for
percentage borders it would give the wrong results there too.

That said, I guess you may as well get it checked in since it fixes something
and it's pretty simple.

Comment 20

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

Filed comment 15 as bug 236966.

Comment 21

[Boris Zbarsky [:bzbarsky]]

David, can you do this checkin before freeze?

Comment 22

[Boris Zbarsky [:bzbarsky]]

Checked in to the trunk for 1.7b.

Hideo Saito, many thanks for the patch!

Comment 23

[Hixie (not reading bugmail)]

> Of course, this still doesn't work for percentage borders

At the moment, borders can't be %-based. (Although indeed we should probably be
future-proofing for the eventual day when someone convinces the WG to allow it.)