Last Comment Bug 453827 - Mathematical Script Small k (U+1D4C2) makes the contents of a textbox invisible by scrolling the baseline out of view
: Mathematical Script Small k (U+1D4C2) makes the contents of a textbox invisib...
Status: RESOLVED FIXED
[sg:low] spoof
: regression, verified1.9.0.14, verified1.9.1
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: Trunk
: x86 Windows XP
: P2 major (vote)
: ---
Assigned To: Karl Tomlinson (:karlt)
:
: Jet Villegas (:jet)
Mentors:
http://www.google.com/search?q=%F0%9D...
: 495179 (view as bug list)
Depends on: 481751
Blocks: CTL-render-textfield 425004
  Show dependency treegraph
 
Reported: 2008-09-05 08:07 PDT by Juan Pablo Lopez Yacubian
Modified: 2009-12-03 16:23 PST (History)
20 users (show)
roc: wanted1.9.1+
dveditz: blocking1.9.0.14+
dveditz: wanted1.9.0.x+
dveditz: wanted1.8.1.x-
karlt: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.3+
.3-fixed


Attachments
Plan of patch (not tested) (2.21 KB, patch)
2009-02-03 08:06 PST, Masahiro YAMADA
no flags Details | Diff | Splinter Review
Plan of patch rev.1.01 (2.21 KB, patch)
2009-02-04 06:32 PST, Masahiro YAMADA
no flags Details | Diff | Splinter Review
Patch plan rev.1.02 (1.31 KB, text/plain)
2009-02-04 07:17 PST, Masahiro YAMADA
no flags Details
screenshot (48.45 KB, image/png)
2009-02-19 04:15 PST, Masahiro YAMADA
no flags Details
screenshot with workaround (53.08 KB, image/png)
2009-02-19 04:17 PST, Masahiro YAMADA
no flags Details
another screenshot (68.63 KB, image/png)
2009-02-19 06:07 PST, Masahiro YAMADA
no flags Details
invisible char now (4.53 KB, image/jpeg)
2009-02-19 06:26 PST, Juan Pablo Lopez Yacubian
no flags Details
screenshot (49.96 KB, image/png)
2009-02-19 06:41 PST, Masahiro YAMADA
no flags Details
testcase (2.11 KB, text/html)
2009-02-19 16:04 PST, Masahiro YAMADA
no flags Details
testcase, Cambria Math has funky metrics (895 bytes, text/html)
2009-02-22 22:11 PST, John Daggett (:jtd)
no flags Details
patch for 1.9.1 (32.32 KB, patch)
2009-08-05 22:37 PDT, Karl Tomlinson (:karlt)
bzbarsky: review+
Details | Diff | Splinter Review
regression tests (3.89 KB, patch)
2009-08-09 21:24 PDT, Karl Tomlinson (:karlt)
no flags Details | Diff | Splinter Review
fix tests (5.64 KB, patch)
2009-08-09 21:25 PDT, Karl Tomlinson (:karlt)
no flags Details | Diff | Splinter Review
patch for 1.9.1 v2 (31.00 KB, patch)
2009-08-09 21:29 PDT, Karl Tomlinson (:karlt)
bzbarsky: review+
samuel.sidler+old: approval1.9.1.3+
Details | Diff | Splinter Review
patch for 1.9.0 (36.46 KB, patch)
2009-08-09 21:33 PDT, Karl Tomlinson (:karlt)
bzbarsky: review+
dbaron: superreview+
samuel.sidler+old: approval1.9.0.14+
Details | Diff | Splinter Review
screenshot PNG file (24.39 KB, image/png)
2009-08-19 15:14 PDT, Murali Nandigama [:murali]
no flags Details
shiretoko screenshot (37.21 KB, image/png)
2009-08-19 15:14 PDT, Murali Nandigama [:murali]
no flags Details

Description Juan Pablo Lopez Yacubian 2008-09-05 08:07:34 PDT
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 2008-09-05 18:36:38 PDT
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 2008-09-05 18:53:06 PDT
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 2008-09-05 19:08:22 PDT
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 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-09-05 22:52:52 PDT

*** This bug has been marked as a duplicate of bug 452979 ***
Comment 5 Juan Pablo Lopez Yacubian 2008-09-06 02:22:39 PDT
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] 2008-09-10 15:26:31 PDT
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) 2008-10-02 09:25:10 PDT
Assigning this to Masahiro since he owns the bug 452979.
Comment 8 Masahiro YAMADA 2008-10-03 07:19:02 PDT
Is this bug means "all surrogate pair should be encoded" ?
Comment 9 Masahiro YAMADA 2008-10-04 03:47:53 PDT
(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 2008-10-04 08:39:51 PDT
𝓀 (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 2008-10-04 09:18:32 PDT
(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 2008-10-09 23:45:12 PDT
Dão , Does address bar support to show surrogate pair on Windows Vista?
Comment 13 Dão Gottwald [:dao] 2008-10-10 01:06:48 PDT
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] 2008-10-10 01:07:13 PDT
(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 2008-11-03 00:37:22 PST
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] (Mozilla Japan) 2008-11-03 09:44:05 PST
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] 2008-11-11 21:38:51 PST
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) 2009-01-05 07:50:03 PST
Masahiro: Any update here?
Comment 19 Masahiro YAMADA 2009-01-06 07:33:58 PST
I'll write patch for (B) of comment #15 later.
Comment 20 Samuel Sidler (old account; do not CC) 2009-02-02 07:42:17 PST
Masahiro, have you had a chance to work on your patch yet?
Comment 21 Masahiro YAMADA 2009-02-03 08:06:52 PST
Created attachment 360316 [details] [diff] [review]
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] 2009-02-03 08:14:12 PST
Which part of that patch addresses 𝓀?
Comment 23 Masahiro YAMADA 2009-02-04 01:50:35 PST
Comment on attachment 360316 [details] [diff] [review]
Plan of patch (not tested)

Sorry, this is not correct file.
Comment 24 Masahiro YAMADA 2009-02-04 06:32:33 PST
Created attachment 360503 [details] [diff] [review]
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] 2009-02-04 06:47:44 PST
Looks like your tree is outdated. losslessDecodeURI already contains the code that you're adding, from bug 452979.
Comment 26 Dão Gottwald [:dao] 2009-02-04 06:50:14 PST
By the way, "Plan of patch" and "Plan of patch rev.1.01" are identical...
Comment 27 Masahiro YAMADA 2009-02-04 07:17:08 PST
Created attachment 360512 [details]
Patch plan rev.1.02
Comment 28 Mike Beltzner [:beltzner, not reading bugmail] 2009-02-09 08:22:28 PST
Does that patch plan need a review?
Comment 29 Johnathan Nightingale [:johnath] 2009-02-17 07:18:07 PST
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 2009-02-17 09:01:08 PST
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] 2009-02-17 09:07:11 PST
(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 2009-02-19 04:12:28 PST
Most simple workaround (but it breaks UI design):
Add this entry to userChrome.css

#urlbar {
  font-family: Cambria Math;
}
Comment 33 Masahiro YAMADA 2009-02-19 04:15:54 PST
Created attachment 363089 [details]
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 2009-02-19 04:17:32 PST
Created attachment 363090 [details]
screenshot with workaround
Comment 35 Masahiro YAMADA 2009-02-19 06:07:40 PST
Created attachment 363100 [details]
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 2009-02-19 06:12:46 PST
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] 2009-02-19 06:23:31 PST
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 2009-02-19 06:26:19 PST
Created attachment 363102 [details]
invisible char now
Comment 39 Juan Pablo Lopez Yacubian 2009-02-19 06:27:17 PST
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 2009-02-19 06:41:30 PST
Created attachment 363105 [details]
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] 2009-02-19 06:46:19 PST
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 2009-02-19 16:03:20 PST
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 2009-02-19 16:04:08 PST
Created attachment 363216 [details]
testcase
Comment 44 Dão Gottwald [:dao] 2009-02-22 06:58:41 PST
(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 2009-02-22 07:20:53 PST
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) 2009-02-22 22:09:03 PST
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) 2009-02-22 22:11:24 PST
Created attachment 363648 [details]
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] (Mozilla Japan) 2009-02-22 23:11:39 PST
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] 2009-02-23 04:50:53 PST
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) 2009-02-23 14:32:14 PST
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) 2009-02-23 14:37:12 PST
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) 2009-02-23 14:40:21 PST
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) 2009-02-23 14:42:06 PST
(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) 2009-02-23 15:22:17 PST
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 [:bz] (still a bit busy) 2009-02-23 16:44:36 PST
> 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) 2009-02-23 17:38:23 PST
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 [:bz] (still a bit busy) 2009-02-23 17:51:18 PST
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) 2009-02-23 17:54:28 PST
(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) 2009-02-23 17:56:05 PST
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 [:bz] (still a bit busy) 2009-02-23 17:56:05 PST
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) 2009-02-23 18:27:24 PST
(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 [:bz] (still a bit busy) 2009-02-23 18:39:54 PST
> 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) 2009-02-23 18:51:13 PST
(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] 2009-02-24 13:45:48 PST
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) 2009-03-20 10:14:14 PDT
Karl: WIll this be fixed by bug 425004? If not, where are you on a patch for this?
Comment 66 Karl Tomlinson (:karlt) 2009-03-20 13:22:04 PDT
(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) 2009-03-25 20:06:53 PDT
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] 2009-04-17 10:54:46 PDT
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) 2009-04-17 12:48:27 PDT
(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) 2009-05-15 11:33:20 PDT
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) 2009-05-15 13:42:01 PDT
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) 2009-05-18 16:59:46 PDT
Fixed by checkin for bug 481751
Comment 73 Karl Tomlinson (:karlt) 2009-05-19 22:27:31 PDT
A test for this landed in bug 481751.
Comment 74 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-05-20 21:41:17 PDT
Taking off the blocker list since 1.9.1 approval was denied in bug 481751.
Comment 75 Daniel Veditz [:dveditz] 2009-06-27 00:08:35 PDT
*** Bug 495179 has been marked as a duplicate of this bug. ***
Comment 76 Daniel Veditz [:dveditz] 2009-06-27 00:13:32 PDT
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) 2009-06-27 01:19:43 PDT
(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) 2009-07-13 13:13:26 PDT
This needs to block 1.9.1.2, but not 1.9.1.1.
Comment 79 Karl Tomlinson (:karlt) 2009-08-05 22:37:47 PDT
Created attachment 392884 [details] [diff] [review]
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) 2009-08-09 20:18:30 PDT
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) 2009-08-09 21:24:03 PDT
Created attachment 393468 [details] [diff] [review]
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) 2009-08-09 21:25:53 PDT
Created attachment 393470 [details] [diff] [review]
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) 2009-08-09 21:29:14 PDT
Created attachment 393472 [details] [diff] [review]
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) 2009-08-09 21:30:58 PDT
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) 2009-08-09 21:33:54 PDT
Created attachment 393473 [details] [diff] [review]
patch for 1.9.0

Essentially the same method as attachment 393472 [details] [diff] [review], but the code differs.
Comment 86 Boris Zbarsky [:bz] (still a bit busy) 2009-08-10 08:01:16 PDT
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) 2009-08-10 14:30:40 PDT
(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) 2009-08-10 14:33:08 PDT
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) 2009-08-10 16:03:19 PDT
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) 2009-08-11 17:40:03 PDT
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5da69f305b74
Comment 91 Samuel Sidler (old account; do not CC) 2009-08-12 00:04:42 PDT
dbaron: We're waiting on review from you here for this last 1.9.0.14 blocker.
Comment 92 David Baron :dbaron: ⌚️UTC-10 2009-08-12 13:57:50 PDT
Comment on attachment 393473 [details] [diff] [review]
patch for 1.9.0

sr=dbaron
Comment 93 Samuel Sidler (old account; do not CC) 2009-08-12 14:03:24 PDT
Comment on attachment 393473 [details] [diff] [review]
patch for 1.9.0

Approved for 1.9.0.14. a=ss
Comment 95 Al Billings [:abillings] 2009-08-18 16:57:49 PDT
How can this be manually tested with the attached testcases?
Comment 96 Karl Tomlinson (:karlt) 2009-08-18 17:35:55 PDT
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 98 Murali Nandigama [:murali] 2009-08-19 15:14:19 PDT
Created attachment 395421 [details]
screenshot PNG file
Comment 99 Murali Nandigama [:murali] 2009-08-19 15:14:58 PDT
Created attachment 395422 [details]
shiretoko screenshot
Comment 100 Al Billings [:abillings] 2009-08-19 15:18:28 PDT
Marking as verified for 1.9.0.14 and 1.9.1.3. Thanks, Murali.

Note You need to log in before you can comment on or make changes to this bug.