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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

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.

Comment 1

[Simon Montagu :smontagu]

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.

Comment 2

[Simon Montagu :smontagu]

Raising severity and requesting blocking1.9 because with the current behaviour the missing glyph boxes are useless in identifying the codepoints without glyphs.

Comment 3

[Simon Montagu :smontagu]

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)

Comment 4

[Simon Montagu :smontagu]

Attachment: Patch

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.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+            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.

Comment 6

[Simon Montagu :smontagu]

(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?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 8

[Simon Montagu :smontagu]

Attachment: Addressed review comments

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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;

Comment 10

[Simon Montagu :smontagu]

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.