Closed Bug 376532 Opened 16 years ago Closed 15 years ago

should show single missing glyph box for non-BMP characters (not surrogates)

Categories

(Core :: Graphics, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: dbaron, Assigned: smontagu)

References

()

Details

Attachments

(1 file, 1 obsolete file)

We currently show two missing glyph boxes for a character not in plane one -- one for each surrogate.  We should show a single missing glyph box with six digits in it.

Steps to reproduce: load data:text/html,𐏿
Expected results: one box, with 010 3FF
Actual results: two boxes, one with DB 00, the second with DF FF.
Blocks: 386570
This has got worse recently, at least on Windows: we now show only one box with the high surrogate, and the low surrogate disappears completely.
Raising severity and requesting blocking1.9 because with the current behaviour the missing glyph boxes are useless in identifying the codepoints without glyphs.
Severity: normal → major
Flags: blocking1.9?
The regression was between 20070610 and 20070611. Maybe bug 332649? (But if we have to work on this anyway I'd rather fix it completely than go back to the previous semi-broken behaviour)
Attached patch Patch (obsolete) — Splinter Review
01
                          010      03
I hesitated a bit between 3FF and  FF for the format of the 6-digit hex box. The second is perhaps more readable, but the first is easier to fit in at small font sizes.
Assignee: nobody → smontagu
Status: NEW → ASSIGNED
Attachment #281474 - Flags: superreview?(roc)
Attachment #281474 - Flags: review?(roc)
+            if (NS_IS_HIGH_SURROGATE(aString[index]) &&
+                NS_IS_LOW_SURROGATE(aString[index + 1])) {

You need to bounds-check index+1, it might not be there.

 static const int MIN_DESIRED_WIDTH =
   BOX_HORIZONTAL_INSET + BOX_BORDER_WIDTH + HEX_CHAR_GAP +
-  MINIFONT_WIDTH + HEX_CHAR_GAP + MINIFONT_WIDTH +
+  MINIFONT_WIDTH + HEX_CHAR_GAP + MINIFONT_WIDTH + HEX_CHAR_GAP + MINIFONT_WIDTH +

This should vary depending on whether we're dealing with a non-BMP character or not.

+    if (aRect.Width() >= 3*MINIFONT_WIDTH + HEX_CHAR_GAP &&

2*HEX_CHAR_GAP now, for non-BMP chars, right?

+            gfxFloat first = -(MINIFONT_WIDTH * 1.5 + HEX_CHAR_GAP);
+            gfxFloat second = -(MINIFONT_WIDTH * 0.5);
+            gfxFloat third = (MINIFONT_WIDTH * 0.5 + HEX_CHAR_GAP);

I slightly prefer division here

Thanks for doing this.
(In reply to comment #5)
>  static const int MIN_DESIRED_WIDTH =
>    BOX_HORIZONTAL_INSET + BOX_BORDER_WIDTH + HEX_CHAR_GAP +
> -  MINIFONT_WIDTH + HEX_CHAR_GAP + MINIFONT_WIDTH +
> +  MINIFONT_WIDTH + HEX_CHAR_GAP + MINIFONT_WIDTH + HEX_CHAR_GAP +
> MINIFONT_WIDTH +
> 
> This should vary depending on whether we're dealing with a non-BMP character or
> not.

This was deliberate, in order to keep the width of the hex-box the same for all characters, but I suppose there aren't often going to be BMP and non-BMP characters in the same text so maybe it's unnecessary?
I tried to keep the width of the hex-box as small as possible so that they're as close as possible to the width of normal characters. I think it'll be OK for non-BMP hexboxes be wider.
Attachment #281474 - Attachment is obsolete: true
Attachment #281530 - Flags: superreview?(roc)
Attachment #281530 - Flags: review?(roc)
Attachment #281474 - Flags: superreview?(roc)
Attachment #281474 - Flags: review?(roc)
Comment on attachment 281530 [details] [diff] [review]
Addressed review comments

+static const int MIN_DESIRED_WIDTH_4_DIGIT =
   BOX_HORIZONTAL_INSET + BOX_BORDER_WIDTH + HEX_CHAR_GAP +
   MINIFONT_WIDTH + HEX_CHAR_GAP + MINIFONT_WIDTH +
   HEX_CHAR_GAP + BOX_BORDER_WIDTH + BOX_HORIZONTAL_INSET;
 
+static const int MIN_DESIRED_WIDTH_6_DIGIT =
+  BOX_HORIZONTAL_INSET + BOX_BORDER_WIDTH + HEX_CHAR_GAP +
+  MINIFONT_WIDTH + HEX_CHAR_GAP + MINIFONT_WIDTH + HEX_CHAR_GAP + MINIFONT_WIDTH +
+  HEX_CHAR_GAP + BOX_BORDER_WIDTH + BOX_HORIZONTAL_INSET;

Move these into GetDesiredMinWidth and share the common stuff:

+  BOX_HORIZONTAL_INSET + BOX_BORDER_WIDTH + HEX_CHAR_GAP +
+  MINIFONT_WIDTH + HEX_CHAR_GAP + MINIFONT_WIDTH + (... ? 0 : HEX_CHAR_GAP + MINIFONT_WIDTH) +
+  HEX_CHAR_GAP + BOX_BORDER_WIDTH + BOX_HORIZONTAL_INSET;
Attachment #281530 - Flags: superreview?(roc)
Attachment #281530 - Flags: superreview+
Attachment #281530 - Flags: review?(roc)
Attachment #281530 - Flags: review+
Attachment #281530 - Flags: approval1.9+
Checked in with reftests to catch the original behaviour showing two surrogates, the regression showing only the high surrogate and (just in case) showing only the low surrogate.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Flags: blocking1.9?
You need to log in before you can comment on or make changes to this bug.