Closed Bug 236175 Opened 20 years ago Closed 20 years ago

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


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

Not set





(Reporter: masayuki, Assigned: hsaito54)



(2 files, 2 obsolete files)

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.

Reproducible: Always
Steps to Reproduce:
1.See next test cases.
test case 1
test case 2

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.

> nsHTMLContainerFrame::PaintTextDecorations
> nsHTMLContainerFrame::PaintTextDecorationLines
> {
>   nsMargin bp;
>   CalcBorderPadding(bp);
>  nscoord innerWidth = mRect.width - bp.left - bp.right;
>   aRenderingContext.FillRect(bp.left, 
>                     + 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.
Attachment #142777 - Flags: superreview?(dbaron)
Attachment #142777 - Flags: review?(dbaron)
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.
Attached patch patch2 (obsolete) — Splinter Review
Can this patch be evaluated?

-    bpad.GetBorderPadding(aBorderPadding);
+    if (!bpad.GetBorderPadding(aBorderPadding)) {
+      const nsStylePadding* paddingStyle = GetStylePadding();
+      paddingStyle->CalcPaddingFor(this, aBorderPadding);
+    }
Attachment #142777 - Attachment is obsolete: true
Attachment #142777 - Flags: superreview?(dbaron)
Attachment #142777 - Flags: review?(dbaron)
Comment on attachment 142887 [details] [diff] [review]

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?
Attachment #142887 - Flags: superreview?(dbaron)
Attachment #142887 - Flags: review+
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.
shared/src/nsStyleStruct.cpp -> content/shared/src/nsStyleStruct.cpp
Hideo Sato, what are you actually trying to say in comment 6?
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) {
          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);
          frame = frame->GetParent();
        result = (nscoord)((float)baseWidth * aCoord.GetPercentValue());
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...
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.
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.
Yes, that was just changed.
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).
Attached patch patch3Splinter Review
Attachment #142887 - Attachment is obsolete: true
Attachment #142887 - Flags: superreview?(dbaron)
Attachment #142887 - Flags: review+
Attachment #143403 - Flags: review?(bzbarsky)
Comment on attachment 143403 [details] [diff] [review]

David is right... we should spend some time and make this whole setup sane.
Attachment #143403 - Flags: review?(bzbarsky) → review+
Comment on attachment 143403 [details] [diff] [review]

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.
Attachment #143403 - Flags: superreview+
David, can you do this checkin before freeze?
Assignee: nobody → saito
Ever confirmed: true
Checked in to the trunk for 1.7b.

Hideo Saito, many thanks for the patch!
Closed: 20 years ago
Resolution: --- → FIXED
> 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.)
You need to log in before you can comment on or make changes to this bug.