Open Bug 228673 Opened 21 years ago Updated 1 year ago

boxObject.height is not being calculated correctly for xul:label with certain styles

Categories

(Core :: XUL, defect)

defect

Tracking

()

People

(Reporter: iannbugzilla, Unassigned)

References

(Blocks 1 open bug, )

Details

(Keywords: testcase, Whiteboard: [reflow-refactor])

Attachments

(5 files, 4 obsolete files)

The above URL was one of the possible patches for bug 67127 but the problem
doron had was the height didn't work. I've been doing some digging around using
the following xul:label and trying to get the height working:

<xul:label style="white-space: -moz-pre-wrap;" class="tooltip-label"
xbl:inherits="xbl:text=label,crop" crop="right" flex="1"> </xul:label>

The problem still is the height does not seem to get calculated correctly. If
you set the height to be the boxObject.height then it gets messed up as this
only seems to get recalculated once the tooltip is displayed and never decreases.
For example, if you have 3 items that will generate tooltips:
item A has a 1 line tooltip
item B has a 2 line tooltip
item C has a 3 line tooltip

If, after starting up mozilla, you mouse over the items in the following order
you get the following results for height of tooltip.
item A - 0 height
item B - 1 height
item A - 2 height
item B - 2 height

Restart again and try this order
item B - 0 height
item A - 2 height
item C - 2 height
item A - 3 height

Should I be trying something other than boxObject.height or is something broke
with that?
I'm not sure what boxObject.height is supposed to do, but I wouldn't be
surprised if this were invalid, and an attached testcase would probably be nice.
Assignee: dbaron → hyatt
Component: Style System (CSS) → XP Toolkit/Widgets: XUL
QA Contact: ian → shrir
Attached patch Patch for testcase (obsolete) — Splinter Review
To demonstrate upcoming testcase need to apply this patch.
Attached file Testcase with 1, 2 and 3 line tooltips (obsolete) —
Testcase shows the three items. The problem isn't as bad as I first thought but
still is a problem. The mousing over in the following orders produce the
heights as below:
Item A produces a box of almost 0 lines in height
Item B produces a box the right size being 2 lines in height
Item C produces a box the right size being 3 lines in height
Item B produces a box one line too big being 3 lines in height
Item A prodcues a box two lines too big being 3 lines in height

Restarting mozilla and doing the following order:
Item B produces a box the right size being 2 lines in height
Item A produces a box one line too big being 2 lines in height
Item C produces a box the right size being 3 lines in height
Item A produces a box two lines too big being 3 lines in height
Attachment #137526 - Attachment is obsolete: true
Summary: boxObject.height is not been calculated correctly for xul:label with certain styles → boxObject.height is not being calculated correctly for xul:label with certain styles
Keywords: testcase
This bug has had no activity for 5 months, and is blocking a 4 year old 'major'
headache of a bug 45375. 

Testcase WFM, correctly as per RFC => title attributes should ignore CR/LF's. 

Kill the block if its not relevant to the dependant bug, so someone can review
the 2 waiting patches in 45375.
(In reply to comment #5)
> Testcase WFM, correctly as per RFC => title attributes should ignore CR/LF's. 
> 
> Kill the block if its not relevant to the dependant bug, so someone can review
> the 2 waiting patches in 45375.

Wait - did you test it WITH attachment #137524 [details] [diff] [review] applied?  It doesn't apply unless
that change is made, which is the same patch from the multiline tooltip bug.  It
changes the xul element used to display tooltips, and the NEW element is the one
that has this problem. (which I can reproduce, but I'm using an older build
since I don't particularly like some of the newer one's bugs.)

-[Unknown]
Requesting blocking1.7
Flags: blocking1.7?
Flags: blocking1.7? → blocking1.7-
Flags: blocking-aviary1.0RC1?
Flags: blocking-aviary1.0RC1? → blocking-aviary1.0RC1-
Since it was unblocked, could it at least get a "target milestone"?
> tipNode.setAttribute("height", tipNode.boxObject.height);

The box object height is calculated from the height of tipNode's box. If you set
the height to some number, then, in this case, boxObject.height will be at least
that size. Perhaps you want to remove the height before setting the label.

I can't get tooltips to work at all with your patch, so I can't test it.
(In reply to comment #9)
> > tipNode.setAttribute("height", tipNode.boxObject.height);
> 
> The box object height is calculated from the height of tipNode's box. If you set
> the height to some number, then, in this case, boxObject.height will be at least
> that size. Perhaps you want to remove the height before setting the label.
> 
> I can't get tooltips to work at all with your patch, so I can't test it.

Please see bug #45375.... I've posted something there about this problem.

I have been able to get tooltips working with this patch, but it required my
mucking around to make it work properly for me.  I think your point about
removing the existing attribute is very valid, but I fear I'm not sure where
you'd do that.

Thanks,
-[Unknown]
boxObject.height is not a setter only a getter.
You would expect removing the label attribute would work but it does not. As
soon as you set the label attribute again the boxObject.height goes back to the
value from before.
It looks like the boxObject.height is cached and not recalculated.
I'm just trying to get my head round GetOffsetRect
http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsBoxObject.cpp#173
as  I think that is where boxObject.height is being calculated.
Let me clarify:

You're doing tipNode.setAttribute("height", tipNode.boxObject.height);

which will result in <tooltip height="50"> (assuming the height of the box is 50).

Removing the label doesn't change the box's height since you explicitly set it
to 50. Adding a label with height 200 will set <tooltip height="200">.

Then, adding a label with height 50 again will still result in a
boxObject.height of 200 since that's what height you set the box to. Meaning, it
won't shrink less than that just because it contains no content. If you remove
the height attribute, the boxobject height will shrink to fit its contents again.

Using the following code:
      tipNode.setAttribute("label", t);
      tipNode.setAttribute("height", tipNode.boxObject.height);
As you say a single line tooltip gives a height of 19, if you move to a two line
tooltip gives a height of 32 and moving back to a single line still has a height
of 32. Moving to a three line tooltip gives 59 (should be 55).

If you use the following code:
      tipNode.removeAttribute("label");
      tipNode.removeAttribute("height");
      tipNode.setAttribute("label", t);
      tipNode.setAttribute("height", tipNode.boxObject.height);

then a single line tooltip gives a height of 19, if you move to a two line
tooltop the height becomes 97! Moving to a three line tooltip gives 149!
If instead of:
tipNode.removeAttribute("height");
you use:
tipNode.setAttribute("height", 0);

then the height becomes 3 for single line tooltips, 97 for two line tooltips and
149 for three line tooltips.
Interestingly, on my Firefox 1.0 (Debian GNU/Linux if that makes any
difference), the tooltips in the testcase are all on one line. The CR character
shows the "000A square" (a square with 000A written in it, that's what happens
when trying to display characters not in the font). So that means the CR is not
interpreted but just tried to be printed out...
(In reply to comment #15)
> Interestingly, on my Firefox 1.0 (Debian GNU/Linux if that makes any
> difference), the tooltips in the testcase are all on one line. The CR character
> shows the "000A square" (a square with 000A written in it, that's what happens
> when trying to display characters not in the font). So that means the CR is not
> interpreted but just tried to be printed out...

(In reply to comment #2)
> Created an attachment (id=137524)
> Patch for testcase
> 
> To demonstrate upcoming testcase need to apply this patch.

Please make sure to read the entire problem; what is happening is that, in
trying to fix the "long tooltips" bug, this problem was found.  You must apply
the patch made to fix the other bug before you will see anything other than
exactly what you reported.

However, I don't know that the original reported problem is a problem.  No
slight to the reporter, who I'm sure we all respect, but I think the *original*
problem was caused by a mistake.  However, there does seem to be a problem - as
I said in bug #45375 comment #147, I was able to get a "working" solution, that
performed with this testcase admirably - but only by hardcoding a multiple for
the height.

For some reason, even removing the height, the height is calculated strangely. 
I think that is the current thrust of the bug, but I fear it doesn't seem to be
getting looked at much anymore.

Again, I found - as I stated in the other bug - that specifying a "default line
height" it would calculate actually correctly.  The problem was that if this was
done for a single-line tooltip, it would be calculated incorrectly.  I'm also
afraid this may have just meant an error in my code, and also know that
hard-coding 19 is not a viable option.

But, the square is character 10 (I think, otherwise it's 13) and that's caused
by it not being pre-wrapped.

-[Unknown]
As per my note in bug 45375 comment #174:

The author of the Popup ALT Attributes
(http://piro.sakura.ne.jp/xul/_popupalt.html.en) extension said that his code
could be used as a starting point to fix this bug, as well as bug 45375 (though
he wasn't aware of these two bugs on bugzilla). He emailed me an explanation of
his code, but  stated that he thought the operations in his code seemed cosmetic
and, as such, a patch based on his code would probably get rejected.
Nevertheless, I am hopeful someone can take this code and work a patch out. I
will attach the relevent part of his email explaining his code...
Attachment #137524 - Attachment is obsolete: true
Attachment #137527 - Attachment is obsolete: true
Assignee: hyatt → nobody
QA Contact: shrir → xptoolkit.xul
What's happening in this case is that the prefered height of the label is being
set to the same as the minimum height in nsFrame::RefreshSizeCache at
http://lxr.mozilla.org/mozilla/source/layout/generic/nsFrame.cpp#4920

The line is metrics->mBlockPrefSize.height  = metrics->mBlockMinSize.height;

Changing that to the desired size makes the tooltips size better although it
breaks most dialogs. I don't know what the height calculation should be doing,
but this is the code responsible so some xul layout guru (if such a thing
exists) might be able to examine further.
At:
http://lxr.mozilla.org/mozilla/source/layout/generic/nsFrame.cpp#4913
Shouldn't this:
            if (lineBounds.height > metrics->mBlockMinSize.height)
              metrics->mBlockMinSize.height = lineBounds.height;
just be this:
              metrics->mBlockMinSize.height += lineBounds.height;
?
I've tried that and it makes the tooltip initially too high (gets better after
second/third/etc visit), and it also breaks most dialogs. This seems to make
tooltips/dialogs too high on every space it encounters in the tooltip/dialog (a
line higher on every space it encounters).
if (lineBounds.height > metrics->mBlockMinSize.height)
  metrics->mBlockMinSize.height = lineBounds.height;

should be

if (lineFlags == 4 || !firstFrame)
  metrics->mBlockMinSize.height += lineBounds.height;

lineFlag of 4 means the line ends in a break, so each of those lines should be
added. The final line might not have a break, so it needs to be added as well.
That logic wouldn't be nessicary if spaces didn't randomly show up as additional
lines. I can't test that fix right now, it makes sense.
Ack, oops, that won't work, I got things out of order there. firstFrame will
always be true on a valid line.
Attached patch Potential FixSplinter Review
This actually makes some sense...could someone who knows how to look at these
things take a look?
Attachment #190912 - Flags: review?
You'll get a quicker response if you set a specific reviewer. I suggest
bzbarsky@mit.edu as he's quite responsive. 

Check the developers page on mozilla.org for the complete list of module owners. 
Attachment #190912 - Flags: review? → review?(bzbarsky)
I'm not going to be able to look at this until at least Aug 14 (if only because
I don't really know this code well enough to review quickly and won't have time
until then to spend a day or two reading code).

That said, the approach seems to be pretty odd; if this code is used anywhere
but the exact place that was tested it's likely to be wrong...
I've had a look at the patch only change I can see would be

PRInt32 -> PRUint32

4 -> NS_LINE_FLAG_ENDS_IN_BREAK

I'm not too sure I understand the count == framesInBox - 1 part, could you
explain that?
(In reply to comment #27)

> 
> I'm not too sure I understand the count == framesInBox - 1 part, could you
> explain that?

The idea is that to calculate the height of the box, we should add the height of
each (visible) line. Therefore, this checks if the line ends in a break, and if
so, adds that height. The last line, however, doesn't end in a break, so it
needs to be added as well. Now that I think about this, it's not quite right,
but certainly closer than the previous code which seemed to assume that the box
is only one line high...
That makes sense now.

I couldn't see any obvious regressions when I patched my local tree.
It's looking like I won't be able to look at this at least until September. 
Robert, David do you know this code at all?

Michael, it'd probably make it a lot easier to review this patch if you
explained why this works to fix this bug (which you mostly did) and why this
doesn't break anything else.
Comment on attachment 190912 [details] [diff] [review]
Potential Fix

No, this is definitely wrong.  There's no way that the _min_ height (which is
what you're messing with here) should be the sum of the heights of all the
lines.	It should be the height of the tallest line, which is what the original
code does.
Attachment #190912 - Flags: review?(bzbarsky) → review-
Whiteboard: [reflow-refactor]
Attached patch Use mBlockPrefSize then? (obsolete) — Splinter Review
Well, this is almost the same patch as Michael's, except the calculation of
metrics->mBlockMinSize.height is kept as it is and
metrics->mBlockPrefSize.height is now used as the sum of the heights.
Not sure if that makes the patch right, but I think it addresses the concern in
comment 31, not?
That seems more likely to be correct, yeah... how does that affect boxes that
have content removed (so that they used to have to line-break but now no longer
have to)?
Attached file testcase2
That seems to work fine (part 2 of this testcase).
The patch fixes part 2 and part 3 of this testcase (and fixes the previous testcase).
It doesn't seem to regress anything further.
It doesn't fix part 1, because this is in the patch:
+          if (lineFlags == 4 || count == framesInBox - 1)
Afaict, this shouldn't be necessary, but removing that makes the problems mentioned in comment 21 appear.
Attachment #200390 - Flags: review?(bzbarsky)
Martijn, I don't think I know this code well enough to mark r+ on this in any sort of reasonable time frame (like in 2005) with any hope of the review being worth anything....

That said, I'd suggest clearly documenting in the patch what the "4" is (is there really no name for that constant?) and why we're only adding some lines into the pref size.
Comment on attachment 200390 [details] [diff] [review]
Use mBlockPrefSize then?

Please find another reviewer (roc or dbaron?)
Attachment #200390 - Flags: review?(bzbarsky)
Attached patch patch3Splinter Review
Ok, replaced the 4 with NS_LINE_FLAG_ENDS_IN_BREAK and added a comment (the comment probably sucks though).
Attachment #200390 - Attachment is obsolete: true
Attachment #200812 - Flags: review?(roc)
> patch3

+  if (lineBounds.height > metrics->mBlockMinSize.height)
+    metrics->mBlockMinSize.height = lineBounds.height;  
+
+  // The following if.. line should not be necessary, but
+  // for lines that contain space we get somehow too high
+  // boxes (tooltips) on first reflow, which seems related
+  // to the amount of spaces the first line contains
+  if (lineFlags == NS_LINE_FLAG_ENDS_IN_BREAK || count == framesInBox - 1)
+    metrics->mBlockPrefSize.height += lineBounds.height;

I don't understand how lineBounds.height could be used to compute mBlockMinSize.height if it's "someshow too high" for computing mBlockPrefSize.height. Either the same if (...) protection should be used for both or it shouldn't be used for either. Otherwise, the mBlockMinSize.height will end up being too high for those cases where first reflow reports too high box. I think that correct fix would be to remove the second if (...) here and fix the issue that lineBounds.height doesn't make sense.
Its not that lineBounds.height is incorrect, the problem is, there are phantom lines that show up. Since mBlockMinSize.height is only concerned with the height of the tallest line this is fine (unless one of these phantom lines was taller which doesn't seem to be the case). Those lines don't end in a break, so the if allows them to be ignored. With that said, it would be nice to figure out where those phantom lines are coming from. 

For anyone who knows about such things, is there a definition somewhere of what the min,pref and max height should be?
if (lineFlags == NS_LINE_FLAG_ENDS_IN_BREAK || count == framesInBox - 1)

should probably be 

if (lineFlags & NS_LINE_FLAG_ENDS_IN_BREAK || count == framesInBox - 1)

in case any other flags happen to be set
+        PRInt32 framesInBox;

This should be "linesInBlock".

I'd like to know more about these phantom lines. Do they have zero lineBounds.width by any chance? If so, it'd be better to use that to eliminate them.

Even better would be to have GetLine() return a flag corresponding to the result of line->CachedIsEmpty() and use that to ignore empty lines in the pref-height and min-height computation, if that actually works.

Also nsILineIterator seems like a worthless and inefficient abstraction. It'd be cool if someone ripped it out and replaced all uses with nsLineList_iterator.
Attachment #200812 - Flags: review?(roc)
(In reply to comment #41)
> I'd like to know more about these phantom lines. Do they have zero
> lineBounds.width by any chance? If so, it'd be better to use that to eliminate
> them.
No, they don't have zero width. The result I get for the first case in https://bugzilla.mozilla.org/attachment.cgi?id=184656 for instance, is:
lineBounds.width: 360
lineBounds.width: 300
lineBounds.width: 525
lineBounds.width: 300

> Even better would be to have GetLine() return a flag corresponding to the
> result of line->CachedIsEmpty() and use that to ignore empty lines in the
> pref-height and min-height computation, if that actually works.
I tried that (at least I think I tried that) with this patch:
http://wargers.org/mozilla/testpatch.txt
It doesn't seem to work, I still get the same problem as mentioned in comment 21.
I think the basic problem here is that we don't know how box-wrapped blocks are supposed to behave.
Martin seems so close.  And interesting testcase. 

Roc, what do you mean by "we don't know how box-wrapped blocks are supposed to behave?"  No precident or spec?
We've got precedent --- a bad one --- which is whatever our code happens to do. We don't have a spec.
Well, currently there is a difference between initial reflow and incremental reflow, which is alway wrong, not?
Yes, that's one reason our current code is a bad precedent.
Blocks: 218223
This bug really needs to be fixed so that te tooltip bug (45375) can be fixed. Since the tooltip bug has been given a severity of "major" and it can't be fixed until this bug is fixed, it is only logical that this bug also be assigned a severity of "major".  Maybe someone with the appropriate rights could increase the severity and priority of this bug.

Please, anyone with the technical ability, fix this bug and the tooltip bug so that the title attribute of tags can be used as it is intended without causing Firefox users to miss out on important information.  I'd try and help fix this bug myself, but I don't know anything about this type of programming.

It is really ashame that Firefox handles tooltips the worst of all browsers (even MSIE does a better job).
This is invalid.  I looked at the boxObjects for the label and the tooltip and their heights are good.  The bug is somewhere else.
(In reply to comment #43)
> I think the basic problem here is that we don't know how box-wrapped blocks are
> supposed to behave.

Now I think I have a pretty good idea how these should work. Unfortunately changing our code to get it "right" is a really big job and depends on the reflow branch.
Blocks: 358452
(In reply to comment #50)
> (In reply to comment #43)
> > I think the basic problem here is that we don't know how box-wrapped blocks are
> > supposed to behave.
> 
> Now I think I have a pretty good idea how these should work. Unfortunately
> changing our code to get it "right" is a really big job and depends on the
> reflow branch.
> 
Seems like the time to cut another new branch or two is upon us. I imagine this will be happening soon, so these sort of problems can be sorted out and worked on.

Was this fixed by the reflow branch landing?
I suspect not. XUL layout still needs to be reworked to be more compatible with regular CSS layout.
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9-
Whiteboard: [reflow-refactor] → [reflow-refactor][wanted-1.9]
Yuk, shame this 2 year old bug still hasn't been fixed.  I'm currently in the process of trying to create an extension that fixes the FF tooltips, and trying to figure out what the Popup ALT attribute tooltip does.  I understand some but not all of it.
Blocks: 357337
Flags: wanted1.9+
Whiteboard: [reflow-refactor][wanted-1.9] → [reflow-refactor]
Flags: wanted1.9-
Flags: wanted1.9+
Flags: wanted-next+
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
No longer blocks: 67127
Time to clear this up. Fix Bug 1508119 Remove XUL: Calendar/Lightning still uses XUL overlays which we've removed from TB

as a start, to get this put to bed once and for all.
Severity: normal → S3

The severity field for this bug is relatively low, S3. However, the bug has 44 votes.
:enndeakin, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Flags: needinfo?(enndeakin)

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.

Flags: needinfo?(enndeakin)
You need to log in before you can comment on or make changes to this bug.