453827 - Mathematical Script Small k (U+1D4C2) makes the contents of a textbox invisible by scrolling the baseline out of view

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P2)

Description

[Juan Pablo Lopez Yacubian]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; es-ES; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; es-ES; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1

The problem is that the address bar can display any type of character (for example: unicode). This means a problem, because there are some strange characters, with a which format generates errors as not displaying the address in the address bar.

Reproducible: Always

Steps to Reproduce:
1-Create a file and rename with a special character.
2-open it and display the address bar.
Actual Results:  
We can see that the file is open and in the address bar, a blank space.


In my opinion, should be limited capacity to interpret the format in the address bar as a possible solution.

	
CHARACTER IN HTML : 𝓀

Lastly while not is a big problem could perhaps be used for phishing attacks.

Comment 1

[Jesse Ruderman]

I believe this is a duplicate of bug 452979.  Juan, I'll give you access to that bug so you can determine whether that's the case.

Comment 2

[Juan Pablo Lopez Yacubian]

The problem is the same, but as said in the last message ,not all characters were covered.



"  -------  Comment #8 From  Masahiro Yamada   2008-09-02 02:50:39 PDT   (-) [reply] -------

(From update of attachment 336248 [details] [diff] [review])
It is needed to fix list of invisible characters.
This patch contains ONLY UI-ASCII defined cntrol characters.
So, other Unicode control charcters (e.g. BOM) is missing.
"


see you!

Comment 3

[Jesse Ruderman]

Not all invisible characters were covered by the *patch* that Masahiro was withdrawing from review.  They're still covered by the bug :)

Btw, the character you mentioned isn't invisible for me; I get a hexbox.

data:text/html,𝓀

Comment 5

[Juan Pablo Lopez Yacubian]

the character, is not invisible, what happens is it occupies more than normal in size. for that reason is not displayed in the address bar.

Comment 6

[Daniel Veditz [:dveditz]]

Reopening and making dependent on bug 452979 instead. I'm slightly worried that a fix for that bug might be verified only with the original description and miss this case.

Comment 7

[Samuel Sidler (old account; do not CC)]

Assigning this to Masahiro since he owns the bug 452979.

Comment 8

[Masahiro YAMADA]

Is this bug means "all surrogate pair should be encoded" ?

Comment 9

[Masahiro YAMADA]

(In reply to comment #8)
> Is this bug means "all surrogate pair should be encoded" ?

I think ,encodeing all surrogate pair is bad,
Because, Fx3.0.3 on Windows Vista can display surrogate pair correctly in location bar if font has metric.

Comment 10

[Masahiro YAMADA]

𝓀 (U+1D4C0) is not control character.
It is italic "k"  of Mathmatical Alphanumrical Symbol.
So, If you use Windows Vista, and You have font that has metric of U+1D4C0,
It will be displayed as italic "k".

If this bug means "If font metric is not found, character should be displayed as % encoded text, not alternative square", approach of patch of bug 452979 is not able to fix this bug because the patch changes browser.js, not native C++ code.


If this bug means "addreass bar should encode character

Comment 11

[Masahiro YAMADA]

(In reply to comment #10)

> 
> If this bug means "addreass bar should encode character

Sorry, sent before edit finished.
I think,  It may be better to fix native C++ code, not browser.js if we want to encode all characters that cannot be displayed .,

Comment 12

[Masahiro YAMADA]

Dão , Does address bar support to show surrogate pair on Windows Vista?

Comment 13

[Dão Gottwald [:dao]]

The character can be shown on Win XP. However, it occupies a huge vertical space, which causes the _entire_ url to become invisible in the location bar.
http://www.google.com/search?q=

Comment 14

[Dão Gottwald [:dao]]

(In reply to comment #13)
> The character can be shown on Win XP. However, it occupies a huge vertical
> space, which causes the _entire_ url to become invisible in the location bar.
> http://www.google.com/search?q=

http://www.google.com/search?q=%F0%9D%93%80

Comment 15

[Masahiro YAMADA]

How should we show these?
(A). Characters defined as visible character in Unicode specification, but font does not have griph of the character. 

(B). Characters defined as invisible or non-character in Unicode specification , but displayed as squared space( e.g. U+fff0 to U+fff8:Unassigned characters).

(B) can be fixed by patch to browser.js. but (A) can not be fixed by browser.js.
It would be better to devide thers two issue.

Comment 16

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

I think that the anonymous div element in an input element should not be scrollable. It should be fixed in another bug first.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Blocking, but only because it's blocking 1.9.0.5 (which seems odd to me)

Comment 18

[Samuel Sidler (old account; do not CC)]

Masahiro: Any update here?

Comment 19

[Masahiro YAMADA]

I'll write patch for (B) of comment #15 later.

Comment 20

[Samuel Sidler (old account; do not CC)]

Masahiro, have you had a chance to work on your patch yet?

Comment 21

[Masahiro YAMADA]

Attachment: Plan of patch (not tested)

I think , if we can detect surrogate pair is supported or not at run time,
it may be better to change this patch like following code.

if surrogatePairSupported {
  encode non characters.
} else {
  encode all surrogate pairs.
}

Comment 22

[Dão Gottwald [:dao]]

Which part of that patch addresses 𝓀?

Comment 23

[Masahiro YAMADA]

Comment on attachment 360316 [details] [diff] [review]
Plan of patch (not tested)

Sorry, this is not correct file.

Comment 24

[Masahiro YAMADA]

Attachment: Plan of patch rev.1.01

This is not full patch.
Because surrogatePairIsSupported is notdefined.
How should I implement it?

Comment 25

[Dão Gottwald [:dao]]

Looks like your tree is outdated. losslessDecodeURI already contains the code that you're adding, from bug 452979.

Comment 26

[Dão Gottwald [:dao]]

By the way, "Plan of patch" and "Plan of patch rev.1.01" are identical...

Comment 27

[Masahiro YAMADA]

Attachment: Patch plan rev.1.02

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

Does that patch plan need a review?

Comment 29

[Johnathan Nightingale [:johnath]]

Masahiro - is this intended to be a complete patch?  Gavin, you reviewed bug 452979, are you a good candidate to review this one?

Comment 30

[Masahiro YAMADA]

surrogatePairIsSupported  in patch plan we can detect Surrogate pair is supported or not easily.
But it is not easy.
Because, On Windows, surrogate pair implementation history is here.

Windows 2000: Supported, but not enabled by registory
Windows XP or later: Supported, enabled by registory by default.
Windows Vista: Some default fonts contains gryph of surrogate pair characters.

So, After Windows 2000, 𝓀 may be visible if surrogair support is enabled and font used at location bar has gryph for it.
So, it depends on font and registory and code of character in URL.

It means "Patch plan rev.1.02" is not good to fix this issue.

Patch should be like following.
> For each character in URL {
>   if (!(character is visible on font of location bar)) {
>     encode it;
>   }
> }

I don't know how should I implement (character is visible on font of location bar).

Can browser.js detect font used to show URL contains gryph of character in URL before rendering URL string ?

If "Yes", plaase tell me how to dedect.
If "No",  I think, to fix at 1.9.0Branch, different approach is needed.

Comment 31

[Dão Gottwald [:dao]]

(In reply to comment #30)
> Patch should be like following.
> > For each character in URL {
> >   if (!(character is visible on font of location bar)) {
> >     encode it;
> >   }
> > }

What is it that you're trying to fix?
The problem that I experienced is that if the character *can* be rendered, it screws up the location bar. Otherwise we get the fallback square, which seems fine.

Comment 32

[Masahiro YAMADA]

Most simple workaround (but it breaks UI design):
Add this entry to userChrome.css

#urlbar {
  font-family: Cambria Math;
}

Comment 33

[Masahiro YAMADA]

Attachment: screenshot

According to this screenshot, this bug is caused by wrong renering of Gecko.
And it seems to be same as Cambria math font handling problems.

Comment 34

[Masahiro YAMADA]

Attachment: screenshot with workaround

Comment 35

[Masahiro YAMADA]

Attachment: another screenshot

This is screenshot on my main PC.
Difference between attachment 363089 [details] is only installed font.
So this is really font handling problem.

Comment 36

[Masahiro YAMADA]

I wrote comments between 8 and 32 seeing attachment 363100 [details], not attachment 363089 [details].

Juan, Dão, which screenshot is same as what you see ?

Comment 37

[Dão Gottwald [:dao]]

I saw attachment 363089 [details], which is what this bug is about. I'm not sure whether the character is expected to be as high as you can see in attachment 363090 [details], but in any case it seems wrong that such a character can push the whole line out of sight. Maybe this bug should be in Core:Editor.

Comment 38

[Juan Pablo Lopez Yacubian]

Attachment: invisible char now

Comment 39

[Juan Pablo Lopez Yacubian]

Hello

When i insert this char now (𝓀) firefox display the real image of char, not like before that disappear text of address bar. ( this in windows xp)

see attach

Comment 40

[Masahiro YAMADA]

Attachment: screenshot

This screenshot contains five problems (marked with red ring).

1.Title bar character broken.
2.Location bar is empty
3.Height of tootlip of tab is incorrent
4.Textbox besides "Google" logo contains a character 𝓀 but it is displayed as empty box
5. height of text that has lightblue background is too high.

and, problem 2 and 4 seems to be same.

Comment 41

[Dão Gottwald [:dao]]

3 and 5 might be correct, depending on what the expected height of that character is.

2 and 4 is why I'd move this to Core:Editor.

Comment 42

[Masahiro YAMADA]

When this bug is reproduced, content-height of location bar is higer than textbox.
If location bar has focus, I can scroll vertically with mousewheel, and I can see URL.

Comment 43

[Masahiro YAMADA]

Attachment: testcase

Comment 44

[Dão Gottwald [:dao]]

(In reply to comment #41)
> 2 and 4 is why I'd move this to Core:Editor.

Or maybe Layout: Form Controls...

Comment 45

[Masahiro YAMADA]

Now, this bug is very different to bug 452979.
I think, It is better to change asignee from me
to someone who is well informed about text rendering of Gecko.

Comment 46

[John Daggett (:jtd)]

My guess here is that the fix is to explicitly set the line-height.  The math symbols used in the testcase appear to only exist in a limited number of fonts.  I'm running on XP but with the Vista C fonts.  Cambria Math has some funky metrics, the max ascent/descent are rather large:

Font: 4a87910 (Cambria Math) size: 0.000000
    emHeight: 12.000000 emAscent: 9.000000 emDescent: 3.000000
    maxAscent: 37.000000 maxDescent: 30.000000 maxAdvance: 52.000000
    internalLeading: 0.000000 externalLeading: 55.000000
    spaceWidth: 3.000000 aveCharWidth: 7.000000 xHeight: 6.000000
    uOff: -1.000000 uSize: 1.000000 stOff: 3.000000 stSize: 1.000000 suOff: 3.000000 suSize: 1.000000

Note the big difference between (emAscent + emDescent) vs. (maxAscent + maxDescent), normally these are relatively close if not the same.  We use max ascent/descent to calculate the line height as I recall, I think the large line height associated with Cambria Math is what's causing this to break.  But you should be able to override the default line height by setting it explicitly.

Comment 47

[John Daggett (:jtd)]

Attachment: testcase, Cambria Math has funky metrics

Note: not sure if Cambria Math is present on most Windows XP systems, it's definitely available under Vista.

Comment 48

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

another solution (not gfx) for this bug:

input element text should be always aligned by the baseline and the text should not be scrolled. but that may make new accessibility problem which is underline position of IME composing text...

Comment 49

[Mike Beltzner [:beltzner, not reading bugmail]]

Karl: can you take a look at this and comment on some of the proposed solutions in comment 46 and onward?

Comment 50

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

IMHO the baseline of the text in a single-line textbox should be locked to the baseline of the line containing the textbox.

Comment 51

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

Er, I guess I agree with comment #48. If the characters and the IME underline can't both fit in the box, the user loses no matter what, so let's not worry about that.

Comment 52

[Jonathan Kew [:jfkthame]]

To me, it seems wrong if we use maxAscent and maxDescent to determine default line spacing (comment #46). I think the appropriate metrics for this would be typoAscender/Descender/LineGap from the OS/2 table, or if those aren't available then ascender/descender from the 'hhea' table. But making changes here could have lots of implications; it would need to be looked at carefully.

Bug #402239 might be related to this, too.

Comment 53

[Jonathan Kew [:jfkthame]]

(In reply to comment #51)
> Er, I guess I agree with comment #48. If the characters and the IME underline
> can't both fit in the box, the user loses no matter what, so let's not worry
> about that.

Where is the IME underline drawn? If it is below the descenders, that's a potential problem; but if it were drawn just below the baseline of the glyphs, possibly passing through descenders, then it should remain visible.

Comment 54

[Karl Tomlinson (:karlt)]

I think (most of) this bug is the same issue as bug 425004.

The initial position of text in input areas seems to have the top of the text
(maximum extents of the glyphs) aligned with the top of the input area.

The input area is scrollable, but only with a wheel mouse (AFAIK), and it is not at all obvious that the area is scrollable.

(In reply to comment #48)
> another solution (not gfx) for this bug:
> 
> input element text should be always aligned by the baseline and the text
> should not be scrolled. but that may make new accessibility problem which is
> underline position of IME composing text...

(In reply to comment #50)
> IMHO the baseline of the text in a single-line textbox should be locked to the
> baseline of the line containing the textbox.

I mostly agree with these suggestions.  The initial position of the input area
should have the baseline aligned (or at least the caret centered).

But I think the area should still be scrollable.  This is sometimes necessary
to check how many accents are stacked on some characters, or just to view some
glyphs to check that they are the right characters.

I'll look into setting the initial position of the text better.
(But layout changes should be given time to bake before landing on 1.9.0.)

(In reply to comment #52)
> To me, it seems wrong if we use maxAscent and maxDescent to determine default
> line spacing (comment #46). I think the appropriate metrics for this would be
> typoAscender/Descender/LineGap from the OS/2 table, or if those aren't
> available then ascender/descender from the 'hhea' table.

Yes, I agree.  Max ascent and max descent are not normally the right metrics
to use in layout.  (See bug 402473.)

> But making changes here could have lots of implications; it would need to be
> looked at carefully.

Mac and Linux use the hhea table metrics, so that has kind of been tested, but
you are right, it can make a considerable difference to layout.

But even when we use different metrics, a very similar situation will result
when there is a tall/high glyph in the text, so the initial positioning of the
text still needs to be resolved.

(In reply to comment #46)
> My guess here is that the fix is to explicitly set the line-height.

When I did an introductory investigation into similar situations (months ago)
I came to the conclusion that it was the ascent/descent not the line-height
that was causing the problem.  Unfortunately ascent and descent cannot be
explicitly set. 

> Cambria Math has some funky metrics, the max ascent/descent are rather
> large:

Yes, there are some very tall glyphs in that font.

(In reply to comment #47)
> Note: not sure if Cambria Math is present on most Windows XP systems, it's
> definitely available under Vista.

Cambria Math will be present on many MS XP systems as it comes with MS
Office 2007 and even some of the MS free viewers for Office 2007 docs.

Comment 55

[Boris Zbarsky [:bzbarsky]]

> The initial position of text in input areas seems to have the top of the text
> (maximum extents of the glyphs) aligned with the top of the input area.

Yes.  And the baseline of the text aligned with the baseline of the line.  That is, the baseline of the <input> is the baseline of the text and it is by default baseline-aligned.  This means the baseline might be below the bottom of the input:

  data:text/html,aaa<input style="font-size: 4em; height: 0.5em" value="CCC">

It's not clear to me how what comment 48 proposes differs, since we _are_ doing baseline alignment.  The only question is where the "window" onto the text the input provides should be placed vertically if the input is too small for the text, right?  It's not clear to me what the proposed solution is.

> The input area is scrollable, but only with a wheel mouse (AFAIK), and it is
> not at all obvious that the area is scrollable.

You mean vertically?  It's trivial to scroll it horizontally with arrow keys, and if we made it non-scrollable people would complain long and loud very very immediately: you wouldn't be able to arrow through the text if it were longer than the input width.

> The initial position of the input area should have the baseline aligned

Which baseline aligned to where?

Comment 56

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

Good point, I had not realized we have this extra degree of freedom about where the <input> itself is positioned. In your testcase:

  data:text/html,aaa<input style="font-size: 4em; height: 0.5em" value="CCC">

IMHO a reasonable approach would be to make the ratio of the ascent to descent of the <input> equal the ratio of the ascent to descent of the font. This would at least ensure that the baseline of the text in the <input> is visible by default.

Comment 57

[Boris Zbarsky [:bzbarsky]]

By "the font" I assume you mean the text inside the input?  That is, take the blockframe inside the input, find its ascent and descent, and position the block inside the input in such a way that the ratios work as you describe.  Then set the baseline of the whole shebang to the baseline of the text?

That would mean weird jiggling if you type characters that aren't in the default font for the input, but right now we get weird jiggling of the text (but not the borders) in that case anyway.

So sounds good to me, I think.

Comment 58

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

(In reply to comment #57)
> By "the font" I assume you mean the text inside the input?  That is, take the
> blockframe inside the input, find its ascent and descent, and position the
> block inside the input in such a way that the ratios work as you describe. 
> Then set the baseline of the whole shebang to the baseline of the text?

Yes.

Comment 59

[John Daggett (:jtd)]

Note that the font in this case is *not* the default font, it is the font found
via system fallback for the U+1D4C0 character, Cambria Math in this case.

Comment 60

[Boris Zbarsky [:bzbarsky]]

The one concern there is that this is probably "easy" to do in HTML: return the ascent as "ask me" and compute it in Reflow() using the above algorithm, while also repositioning the block vertically as needed.

For XUL, though, we have to deal with the box methods, and in particular I suspect nsSprocketLayout cares about GetBoxAscent really working and not just returning "ask me", right?

Comment 61

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

(In reply to comment #60)
> The one concern there is that this is probably "easy" to do in HTML: return the
> ascent as "ask me" and compute it in Reflow() using the above algorithm, while
> also repositioning the block vertically as needed.

Can't we just return it in the reflow metrics? Seems to me that by the end of nsTextControlFrame::Reflow we know everything we need to compute it.

> For XUL, though, we have to deal with the box methods, and in particular I
> suspect nsSprocketLayout cares about GetBoxAscent really working and not just
> returning "ask me", right?

Yeah :-(.

For XUL, nsTextControlFrame::GetBoxAscent currently seems to try to go and get the ascent of the inner text block. That gets used for the ascent in Reflow() as well. I'm not sure why that gives us the positioning we see in your testcase.

Comment 62

[Boris Zbarsky [:bzbarsky]]

> Can't we just return it in the reflow metrics?

For nsIFrame reflow, exactly.

The positioning in the testcase is a combination of two things:

1) The ascent (as computed by GetBoxAscent).
2) The behavior of nsStackFrame when its content is taller than it (align to top
   and chop off the bottom).

Comment 63

[Karl Tomlinson (:karlt)]

(In reply to comment #55)
> > The initial position of text in input areas seems to have the top of the text
> > (maximum extents of the glyphs) aligned with the top of the input area.
> 
> Yes.  And the baseline of the text aligned with the baseline of the line.  
> That is, the baseline of the <input> is the baseline of the text and it is
> by default baseline-aligned.

I think the issue here is that the distance within the <input> from the top of
the text to the baseline is different from the distance from the top of the
window to the external baseline of <input> shell (as seen by the parent of the
<input>).

So it is not possible for both the tops and the baselines to be aligned.

> This means the baseline might be below the bottom of
> the input:
> 
>   data:text/html,aaa<input style="font-size: 4em; height: 0.5em" value="CCC">
> 
> It's not clear to me how what comment 48 proposes differs, since we _are_ doing
> baseline alignment.  The only question is where the "window" onto the text the
> input provides should be placed vertically if the input is too small for the
> text, right?

Right.  But this doesn't only happen when a small height is specified.

> It's not clear to me what the proposed solution is.
> 
> > The input area is scrollable, but only with a wheel mouse (AFAIK), and it is
> > not at all obvious that the area is scrollable.
> 
> You mean vertically?

Yes.

> > The initial position of the input area should have the baseline aligned
> 
> Which baseline aligned to where?

The window onto the text should be initially placed so that the baseline of the text and line through the window (first baseline I guess, but can there be more than one line?) is aligned with the baseline of the <input> shell (as seen from the parent element).

(In reply to comment #62)

> 2) The behavior of nsStackFrame when its content is taller than it (align to
>    top and chop off the bottom).

That was what I thought was aligning the tops, so the baselines are not aligned.

(In reply to comment #57)
> By "the font" I assume you mean the text inside the input?  That is, take the
> blockframe inside the input, find its ascent and descent, and position the
> block inside the input in such a way that the ratios work as you describe. 
> Then set the baseline of the whole shebang to the baseline of the text?
> 
> That would mean weird jiggling if you type characters that aren't in the
> default font for the input, but right now we get weird jiggling of the text
> (but not the borders) in that case anyway.

I don't think we want the location bar frame jiggling around as we type do we?
Maybe practical, but not pretty.

Comment 64

[Johnathan Nightingale [:johnath]]

Based on IRC conversation with Karl, moving this to Layout:Form Controls.  I'll re-nom too, since it's going to lose its flags.

Comment 65

[Samuel Sidler (old account; do not CC)]

Karl: WIll this be fixed by bug 425004? If not, where are you on a patch for this?

Comment 66

[Karl Tomlinson (:karlt)]

(In reply to comment #65)
> Karl: WIll this be fixed by bug 425004?

Yes, the attachment 368197 [details] [diff] [review] fixes this.  It would probably be best taken with the attachment 367950 [details] [diff] [review] in bug 483558 (once they have baked on trunk), or attachment 368197 [details] [diff] [review] will make bug 483558 more obvious.

Attachment 368197 [details] [diff] didn't actually fix bug 425004 as well as I thought due to the nature of the fonts involved there.  We may be able to do something better or something in addition to attachment 368197 [details] [diff] [review], but I'm not yet clear what, and attachment 368197 [details] [diff] [review] is at least an improvement.

Comment 67

[Karl Tomlinson (:karlt)]

An updated version of 368197 is in bug 481751, but

(In reply to comment #46)
> My guess here is that the fix is to explicitly set the line-height.

I'm thinking I should have paid more attention to John's suggestion.

Setting the line-height explicitly through setting the property on the control is complicated by bug 349259 (indirectly), but having the control set the line-height on its child anonymous div seems appealing.

Comment 68

[Daniel Veditz [:dveditz]]

Please request approval for the patch you want to take for this on the 1.9.0 branch (the one in bug 481751, right?)

Comment 69

[Karl Tomlinson (:karlt)]

(In reply to comment #68)
> Please request approval for the patch you want to take for this on the 1.9.0
> branch (the one in bug 481751, right?)

Yes, I think attachment 373038 [details] [diff] [review] is the best way to fix this, but I still need to write tests, get review, bake on trunk.  I don't think it would be appropriate to land on 1.9.0 without exposure on trunk.

Comment 70

[Damon Sicore (:damons)]

Roc, you still think this should block this late in the game?  Not sure why we're blocking here.

Comment 71

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

We're blocking because it's a security issue. In theory this could be used for spoofing, although personally I'm a bit fuzzy on exactly how. dveditz can probably make the case. We have a patch in bug 481751, it just needs review from bz and landing.

Comment 72

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

Fixed by checkin for bug 481751

Comment 73

[Karl Tomlinson (:karlt)]

A test for this landed in bug 481751.

Comment 74

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

Taking off the blocker list since 1.9.1 approval was denied in bug 481751.

Comment 76

[Daniel Veditz [:dveditz]]

Alternate character that also causes the problem on Windows from bug 495179:
    http://www.google.com/search?q=$E2%9F%B5

Why "long leftward arrow" needs a line height 4x bigger than normal letters is beyond me, it's a very short glyph.

Comment 77

[Jonathan Kew [:jfkthame]]

(In reply to comment #76)
> Why "long leftward arrow" needs a line height 4x bigger than normal letters is
> beyond me, it's a very short glyph.

The issue isn't the size of the particular glyph, but the height/depth of the font that gets used. U+27F5 "long leftward arrow" isn't supported in normal text fonts, and so font fallback ends up using Cambria Math to render it.

Comment 78

[Samuel Sidler (old account; do not CC)]

This needs to block 1.9.1.2, but not 1.9.1.1.

Comment 79

[Karl Tomlinson (:karlt)]

Attachment: patch for 1.9.1

Bug 481751 was a change in behavior that I don't think we want on a
security/stability release (and I haven't yet fixed bug 494901) so I think we
want a smaller change to fix this bug on the release branches.

This is a patch for 1.9.1.x.  It is essentially the code of attachment 378147 [details] [diff] [review]
with the fix for the regression in bug 495959, but it does not include the
change to forms.css to use the code for every text input element.  Instead the
code is only enabled for the urlbar.

I would have liked to set the style for the urlbar in browser.css but we don't
allow the style for native anonymous content to be set from document style
sheets, so I've set the style in xul.css.  I used .chromeclass-location
(rather than #urlbar) as it already is used in xul.css.

(I'm attaching the patch to this bug not bug 481751 because the patch does not
fix bug 481751 and I think this bug is the one we want fixed on release
branches.)

Comment 80

[Samuel Sidler (old account; do not CC)]

Comment on attachment 392884 [details] [diff] [review]
patch for 1.9.1

Assuming bz's review is all that's needed...

Approved for 1.9.1.3. a=ss

Comment 81

[Karl Tomlinson (:karlt)]

Attachment: regression tests

These don't test the fix for this bug but test that we don't end up with bug 494901 or bug 495959.  The patch applies to 1.9.1 but the same tests can be added to 1.9.0 (with some editing of reftest.list).

Comment 82

[Karl Tomlinson (:karlt)]

Attachment: fix tests

These test that this bug is fixed.  The tests use @font-face so the tests can only be run on 1.9.1 (unless the necessary fonts are installed).

Comment 83

[Karl Tomlinson (:karlt)]

Attachment: patch for 1.9.1 v2

I should have removed the nsTextControlFrame::GetBoxAscent() changes from the fix for bug 481751.  Otherwise this is the same as attachment 393468 [details] [diff] [review].

Comment 84

[Karl Tomlinson (:karlt)]

Comment on attachment 392884 [details] [diff] [review]
patch for 1.9.1

(In reply to comment #83)
> Otherwise this is the same as attachment 393468 [details] [diff] [review].

I meant otherwise same as attachment 392884 [details] [diff] [review].

Comment 85

[Karl Tomlinson (:karlt)]

Attachment: patch for 1.9.0

Essentially the same method as attachment 393472 [details] [diff] [review], but the code differs.

Comment 86

[Boris Zbarsky [:bzbarsky]]

So what are the differences between the 1.9.0 patch and the 1.9.1 patch?  Or should I just be rereviwing the 1.9.0 patch in its entirety?

Comment 87

[Karl Tomlinson (:karlt)]

(In reply to comment #86)
> So what are the differences between the 1.9.0 patch and the 1.9.1 patch?  Or
> should I just be rereviwing the 1.9.0 patch in its entirety?

It should be enough to just check the CalcLineHeight implementation, thanks, and that I haven't done anything stupid in changing the callers (which is about half the patch).  It's not really very different - just I had to hand edit much of the code again.

I requested dbaron's sr for his knowledge re whether anything else may have changed between 1.9.0 and 1.9.1 that might interact badly with the changes here.

Comment 88

[Karl Tomlinson (:karlt)]

Comment on attachment 393468 [details] [diff] [review]
regression tests

This can land now, but attachment 393470 [details] [diff] [review] should probably wait until after the fix is released.

Comment 89

[Samuel Sidler (old account; do not CC)]

Comment on attachment 393472 [details] [diff] [review]
patch for 1.9.1 v2

Approved for 1.9.1.3. a=ss

The tests don't need approval to land and should, as you said, land after the fix here has shipped in Firefox 3.5.3.

Comment 90

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5da69f305b74

Comment 91

[Samuel Sidler (old account; do not CC)]

dbaron: We're waiting on review from you here for this last 1.9.0.14 blocker.

Comment 92

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

Comment on attachment 393473 [details] [diff] [review]
patch for 1.9.0

sr=dbaron

Comment 93

[Samuel Sidler (old account; do not CC)]

Comment on attachment 393473 [details] [diff] [review]
patch for 1.9.0

Approved for 1.9.0.14. a=ss

Comment 94

[Karl Tomlinson (:karlt)]

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1250112606&maxdate=1250112671&who=karlt%2B%25karlt.net

Comment 95

[Al Billings [:abillings - ex-MoCo]]

How can this be manually tested with the attached testcases?

Comment 96

[Karl Tomlinson (:karlt)]

Load the URL on Vista or XP with Office 2007 and check that the url is visible in the urlbar.
(The html text input will not show the character on branches, though.)

Comment 97

[Al Billings [:abillings - ex-MoCo]]

Adding Murali since he runs Windows and has Office 2007.

Murali, please use:

ftp://ftp.mozilla.org/pub/firefox/nightly/2009-08-17-04-mozilla-1.9.1/firefox-3.5.3pre.en-US.win32.installer.exe

and

ftp://ftp.mozilla.org/pub/firefox/nightly/2009-08-13-05-mozilla1.9.0/firefox-3.0.14pre.en-US.win32.installer.exe

for verification.

Comment 98

[Murali Nandigama [:murali]]

Attachment: screenshot PNG file

Comment 99

[Murali Nandigama [:murali]]

Attachment: shiretoko screenshot

Comment 100

[Al Billings [:abillings - ex-MoCo]]

Marking as verified for 1.9.0.14 and 1.9.1.3. Thanks, Murali.

Comment 101

[Karl Tomlinson (:karlt)]

Checked in tests from attachment 393468 [details] [diff] [review] on 1.9.0 and 1.9.1 and tests from attachment 393470 [details] [diff] [review] on 1.9.1 only.

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1253089190&maxdate=1253089332&who=karlt%2B%25karlt.net
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/80affd399497