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

RESOLVED FIXED

Status

()

Core
Layout: Form Controls
P2
major
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Juan Pablo Lopez Yacubian, Assigned: karlt)

Tracking

(Blocks: 1 bug, {regression, verified1.9.0.14, verified1.9.1})

Trunk
x86
Windows XP
regression, verified1.9.0.14, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.1 +
blocking1.9.0.14 +
wanted1.9.0.x +
wanted1.8.1.x -
in-testsuite +

Firefox Tracking Flags

(blocking1.9.1 .3+, status1.9.1 .3-fixed)

Details

(Whiteboard: [sg:low] spoof, URL)

Attachments

(12 attachments, 5 obsolete attachments)

53.08 KB, image/png
Details
68.63 KB, image/png
Details
4.53 KB, image/jpeg
Details
49.96 KB, image/png
Details
2.11 KB, text/html
Details
895 bytes, text/html
Details
3.89 KB, patch
Details | Diff | Splinter Review
5.64 KB, patch
Details | Diff | Splinter Review
31.00 KB, patch
Samuel Sidler (old account; do not CC)
: approval1.9.1.3+
Details | Diff | Splinter Review
36.46 KB, patch
dbaron
: superreview+
Samuel Sidler (old account; do not CC)
: approval1.9.0.14+
Details | Diff | Splinter Review
24.39 KB, image/png
Details
37.21 KB, image/png
Details
(Reporter)

Description

9 years ago
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

9 years ago
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.
(Reporter)

Comment 2

9 years ago
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

9 years ago
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,𝓀
Status: UNCONFIRMED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → DUPLICATE
Whiteboard: [sg:dupe 452979]
Duplicate of bug: 452979
(Reporter)

Comment 5

9 years ago
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.
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.
Status: RESOLVED → UNCONFIRMED
Depends on: 452979
Flags: blocking1.9.0.3+
Flags: blocking-firefox3.1?
Resolution: DUPLICATE → ---
Assigning this to Masahiro since he owns the bug 452979.
Assignee: nobody → masa141421356

Comment 8

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

Comment 9

9 years ago
(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

9 years ago
𝓀 (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

9 years ago
(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 .,
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [sg:dupe 452979] → [sg:low] spoof

Comment 12

9 years ago
Dão , Does address bar support to show surrogate pair on Windows Vista?
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=
(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

Updated

9 years ago
Severity: normal → major
Flags: wanted1.9.0.x+
Flags: wanted1.8.1.x-
Keywords: regression
Flags: blocking1.9.0.4+ → blocking1.9.0.5+
Whiteboard: [sg:low] spoof → [sg:low] spoof, probably fixed in bug 452979

Comment 15

9 years ago
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.
I think that the anonymous div element in an input element should not be scrollable. It should be fixed in another bug first.
Blocking, but only because it's blocking 1.9.0.5 (which seems odd to me)
Flags: blocking-firefox3.1? → blocking-firefox3.1+
Target Milestone: --- → Firefox 3.1
Whiteboard: [sg:low] spoof, probably fixed in bug 452979 → [sg:low] spoof [needs patch]
Flags: blocking1.9.0.5+ → blocking1.9.0.6+
Masahiro: Any update here?
Flags: blocking1.9.0.6+ → blocking1.9.0.7+

Comment 19

9 years ago
I'll write patch for (B) of comment #15 later.
Priority: -- → P2
Masahiro, have you had a chance to work on your patch yet?
Flags: blocking1.9.0.7+ → blocking1.9.0.8+

Comment 21

9 years ago
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.
}
Which part of that patch addresses 𝓀?

Comment 23

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

Sorry, this is not correct file.
Attachment #360316 - Attachment is obsolete: true

Comment 24

9 years ago
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?
Looks like your tree is outdated. losslessDecodeURI already contains the code that you're adding, from bug 452979.

Updated

9 years ago
Attachment #360503 - Attachment is patch: true

Updated

9 years ago
Attachment #360316 - Attachment is patch: true
By the way, "Plan of patch" and "Plan of patch rev.1.01" are identical...

Comment 27

9 years ago
Created attachment 360512 [details]
Patch plan rev.1.02
Attachment #360503 - Attachment is obsolete: true
Does that patch plan need a review?
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

9 years ago
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.
(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.

Updated

9 years ago
Attachment #360512 - Attachment is patch: true
Whiteboard: [sg:low] spoof [needs patch] → [sg:low] spoof [needs patch][to be fixed for 1.9.0.8]

Updated

9 years ago
Attachment #360512 - Attachment is obsolete: true
Attachment #360512 - Attachment is patch: false

Comment 32

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

#urlbar {
  font-family: Cambria Math;
}

Comment 33

9 years ago
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

9 years ago
Created attachment 363090 [details]
screenshot with workaround

Comment 35

9 years ago
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

9 years ago
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 ?
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.
Component: Security → Location Bar and Autocomplete
No longer depends on: 452979
QA Contact: firefox → location.bar
Summary: unicode character hidden address bar → Mathematical Script Small k (U+1D4C2) in the location bar makes the entire URL invisible
(Reporter)

Comment 38

9 years ago
Created attachment 363102 [details]
invisible char now
(Reporter)

Comment 39

9 years ago
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
Component: Location Bar and Autocomplete → Security
Depends on: 452979
Summary: Mathematical Script Small k (U+1D4C2) in the location bar makes the entire URL invisible → unicode character hidden address bar

Updated

9 years ago
Component: Security → Location Bar and Autocomplete
No longer depends on: 452979
Summary: unicode character hidden address bar → Mathematical Script Small k (U+1D4C2) makes the entire URL invisible

Comment 40

9 years ago
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.
Attachment #363089 - Attachment is obsolete: true
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

9 years ago
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

9 years ago
Created attachment 363216 [details]
testcase
(In reply to comment #41)
> 2 and 4 is why I'd move this to Core:Editor.

Or maybe Layout: Form Controls...

Comment 45

9 years ago
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.

Updated

9 years ago
Assignee: masa141421356 → nobody

Comment 46

9 years ago
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

9 years ago
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.
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...
Karl: can you take a look at this and comment on some of the proposed solutions in comment 46 and onward?
IMHO the baseline of the text in a single-line textbox should be locked to the baseline of the line containing the textbox.
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.
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.
(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.
Summary: Mathematical Script Small k (U+1D4C2) makes the entire URL invisible → Mathematical Script Small k (U+1D4C2) makes the contents of a textbox invisible by scrolling the baseline out of view
(Assignee)

Comment 54

9 years ago
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.
Assignee: nobody → mozbugz
Status: NEW → ASSIGNED
> 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?
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.
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.
(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

9 years ago
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.
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?
(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.
> 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).
(Assignee)

Comment 63

9 years ago
(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.
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.
Assignee: mozbugz → nobody
Component: Location Bar and Autocomplete → Layout: Form Controls
Flags: blocking-firefox3.1+
Product: Firefox → Core
QA Contact: location.bar → layout.form-controls
Target Milestone: Firefox 3.1 → ---
Version: unspecified → Trunk
Flags: blocking1.9.1?
Assignee: nobody → mozbugz
Flags: blocking1.9.1? → blocking1.9.1+
Flags: blocking1.9.0.8+ → blocking1.9.0.9?
Whiteboard: [sg:low] spoof [needs patch][to be fixed for 1.9.0.8] → [sg:low] spoof [needs patch]
Karl: WIll this be fixed by bug 425004? If not, where are you on a patch for this?
(Assignee)

Comment 66

8 years ago
(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.
Whiteboard: [sg:low] spoof [needs patch] → [sg:low] spoof [patch in bug 425004]
Depends on: 483558, 425004
(Assignee)

Updated

8 years ago
Depends on: 481751
(Assignee)

Comment 67

8 years ago
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.
Whiteboard: [sg:low] spoof [patch in bug 425004] → [sg:low] spoof [patch in bug 481751]
Flags: blocking1.9.0.10? → blocking1.9.0.10+
Please request approval for the patch you want to take for this on the 1.9.0 branch (the one in bug 481751, right?)
(Assignee)

Comment 69

8 years ago
(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.
Flags: blocking1.9.0.10+ → blocking1.9.0.11?
Roc, you still think this should block this late in the game?  Not sure why we're blocking here.
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.
Fixed by checkin for bug 481751
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 73

8 years ago
A test for this landed in bug 481751.
Flags: in-testsuite+
Whiteboard: [sg:low] spoof [patch in bug 481751] → [sg:low] spoof [fixed in bug 481751][191 landing depends on approval]
(Assignee)

Updated

8 years ago
Blocks: 425004
No longer depends on: 425004, 483558
Whiteboard: [sg:low] spoof [fixed in bug 481751][191 landing depends on approval] → [sg:low] spoof [fixed in bug 481751][191 approval denied]
Taking off the blocker list since 1.9.1 approval was denied in bug 481751.
Flags: blocking1.9.1+ → wanted1.9.1+
Flags: wanted1.9.1.x?
Flags: wanted1.9.1.x?
Flags: wanted1.9.1.x+
Flags: blocking1.9.0.12?
Flags: blocking1.9.0.12+
Flags: blocking1.9.0.12+ → blocking1.9.0.13?
Flags: blocking1.9.1.1?
Duplicate of this bug: 495179
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.
(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.
Flags: blocking1.9.0.13? → blocking1.9.0.13+
This needs to block 1.9.1.2, but not 1.9.1.1.
Flags: blocking1.9.1.1?
Whiteboard: [sg:low] spoof [fixed in bug 481751][191 approval denied] → [sg:low] spoof [1.9.1.2+][fixed in bug 481751][191 approval denied]
blocking1.9.1: --- → .2+
status1.9.1: --- → wanted
blocking1.9.1: .2+ → .3+
Whiteboard: [sg:low] spoof [1.9.1.2+][fixed in bug 481751][191 approval denied] → [sg:low] spoof [fixed in bug 481751]
Flags: wanted1.9.1.x+
(Assignee)

Comment 79

8 years ago
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.)
Attachment #392884 - Flags: review?(bzbarsky)
Attachment #392884 - Flags: review?(bzbarsky) → review+
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
Attachment #392884 - Flags: approval1.9.1.3+
(Assignee)

Comment 81

8 years ago
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).
(Assignee)

Comment 82

8 years ago
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).
(Assignee)

Comment 83

8 years ago
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].
Attachment #393468 - Attachment is obsolete: true
Attachment #393472 - Flags: review?(bzbarsky)
(Assignee)

Updated

8 years ago
Attachment #393468 - Attachment is obsolete: false
(Assignee)

Updated

8 years ago
Attachment #392884 - Attachment is obsolete: true
(Assignee)

Comment 84

8 years ago
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].
(Assignee)

Comment 85

8 years ago
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.
Attachment #393473 - Flags: superreview?(dbaron)
Attachment #393473 - Flags: review?(bzbarsky)
Attachment #393472 - Flags: review?(bzbarsky) → review+
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?
(Assignee)

Comment 87

8 years ago
(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.
(Assignee)

Updated

8 years ago
Attachment #393472 - Flags: approval1.9.1.3?
(Assignee)

Comment 88

8 years ago
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.
Attachment #393468 - Flags: approval1.9.1.3?
Attachment #392884 - Flags: approval1.9.1.3+
Attachment #393472 - Flags: approval1.9.1.3? → approval1.9.1.3+
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.
(Assignee)

Comment 90

8 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5da69f305b74
status1.9.1: wanted → .3-fixed
(Assignee)

Updated

8 years ago
Attachment #393468 - Flags: approval1.9.1.3?
Attachment #393473 - Flags: review?(bzbarsky) → review+
dbaron: We're waiting on review from you here for this last 1.9.0.14 blocker.
Whiteboard: [sg:low] spoof [fixed in bug 481751] → [sg:low] spoof
Attachment #393473 - Flags: superreview?(dbaron) → superreview+
Comment on attachment 393473 [details] [diff] [review]
patch for 1.9.0

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

Approved for 1.9.0.14. a=ss
Attachment #393473 - Flags: approval1.9.0.14+
(Assignee)

Comment 94

8 years ago
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1250112606&maxdate=1250112671&who=karlt%2B%25karlt.net
Keywords: fixed1.9.0.14
How can this be manually tested with the attached testcases?
(Assignee)

Comment 96

8 years ago
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.)
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.
Created attachment 395421 [details]
screenshot PNG file
Created attachment 395422 [details]
shiretoko screenshot
Marking as verified for 1.9.0.14 and 1.9.1.3. Thanks, Murali.
Keywords: fixed1.9.0.14 → verified1.9.0.14, verified1.9.1
Group: core-security
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
(Assignee)

Updated

8 years ago
Blocks: 157541
You need to log in before you can comment on or make changes to this bug.