Oana Botisan, Desktop QA Reporter
	Description • 6 years ago

[Affected versions]:
- Nighlty 60.0a1
- beta 59.0b8
- Firefox 58.0.1

[Affected platforms]:
- Windows 10 x64
- Ubuntu 16.04 x64
- macOS 10.13 

[Steps to reproduce]:
1. Access https://bug1279715.bmoattachments.org/attachment.cgi?id=8762304
2. Open Find Toolbar 
3. Search for the word "Firefox".

[Expected result]:
- The matching words are highlighted.

[Actual result]:
- The word is not highlighted and the Find Toolbar input field is highlighted with red. 

[Regression range]:
- Last good build: 2017-10-09
- First bad build: 2017-10-10
MOzregression concluded that bug 1270140 is the one that might have caused this issue. 

{Additional Notes]:
- This doesn't happen for all the vertical texts. For example, the Find Toolbar works when using this link: https://davidwalsh.name/demo/css-vertical-text.php.

	Mike Taylor [:miketaylr]
	Comment 1 • 6 years ago

Note: once you hit enter/return, the text is highlighted.

The regressing bug seems incorrect, but I don't know the guts of the find toolbar.

	Oana Botisan, Desktop QA Reporter
	Comment 2 • 6 years ago

I am not sure that bug 1270140 cause this issue, but the last good and first bad builds are correct. On the build from 20107-10-09 the word was highlighted as you typed it. 

Also, I've noticed that the word is highlighted if you press enter, but only happens if you write the entire word. If you only type "fire" for example, instead of "firefox", the word is not highlighted.

	Mike de Boer [:mikedeboer]
	Comment 3 • 6 years ago

Brad, might this be something that regressed recently with the visibility checks you implemented? This might be another edge case we'd need to take care of.

	Brad Werth [:bradwerth] Assignee
	Comment 4 • 6 years ago

Sorry for the delay in responding to this. The root cause is that nsRange::GetPartialTextRect doesn't currently handle vertically oriented text frames. This causes the target text to be reported as not visible and thus rejected. I'll work on a fix.

	Brad Werth [:bradwerth] Assignee
	Comment 5 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a917accb0af61a85b08a6f1a68511f19813c93e8

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 6 years ago
mozreview-review

Comment on attachment 8957342 [details]
Bug 1437509 Part 1: Expand nsRange::ExtractRectFromOffset to correctly generate rects from vertical text runs.

https://reviewboard.mozilla.org/r/226260/#review233656

::: dom/base/nsRange.cpp:3236
(Diff revision 1)
>  
>    if (aClampToEdge) {
>      point = aR->ClampPoint(point);
>    }
>  
>    if (aKeepLeft) {

Does aKeepLeft still make sense when the textrun is vertical?  In particular, it basically reflects IsRightToLeft() on the textrun.  Can that be true when IsVertical() is true?

r=me, but the naming of aKeepLeft might need updating if it _does_ apply to vertical text situations.

	Brad Werth [:bradwerth] Assignee
	Comment 11 • 6 years ago

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #8)
> Comment on attachment 8957342 [details]
> Does aKeepLeft still make sense when the textrun is vertical?  In
> particular, it basically reflects IsRightToLeft() on the textrun.  Can that
> be true when IsVertical() is true?
> 
> r=me, but the naming of aKeepLeft might need updating if it _does_ apply to
> vertical text situations.

I thought about it further and realized that aKeepLeft meant more like "of the two potential rects defined by a point inside this rect, do we want the one on the leading edge?". I've updated the code and added comments to make this more clear.

	Mike de Boer [:mikedeboer]
	Comment 12 • 6 years ago
mozreview-review

Comment on attachment 8957343 [details]
Bug 1437509 Part 2: Add a test to ensure we can find vertical text.

https://reviewboard.mozilla.org/r/226262/#review237920

Great! Thanks so much, Brad! I apologize for the super late review, but I was on holiday for a long time :-S Next time, feel free to hop on to #developers of #fx-team and ask who can do a review in my stead. Because I hate to see you having to wait this long...

	Pulsebot
	Comment 13 • 6 years ago

Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0a91126bb0f9
Part 1: Expand nsRange::ExtractRectFromOffset to correctly generate rects from vertical text runs. r=bz
https://hg.mozilla.org/integration/autoland/rev/fe9a67a7ff0d
Part 2: Add a test to ensure we can find vertical text. r=mikedeboer

	Andrei Ciure[:aciure]
	Comment 14 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/0a91126bb0f9
https://hg.mozilla.org/mozilla-central/rev/fe9a67a7ff0d

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 6 years ago

Is there a user impact here which justifies uplift consideration or can this ride the 61 train?

	Brad Werth [:bradwerth] Assignee
	Comment 16 • 6 years ago

(In reply to Ryan VanderMeulen [:RyanVM] from comment #15)
> Is there a user impact here which justifies uplift consideration or can this
> ride the 61 train?

It definitely impacts users browsing vertical writing modes. I'll request uplift.

	Brad Werth [:bradwerth] Assignee
	Comment 17 • 6 years ago

Comment on attachment 8957342 [details]
Bug 1437509 Part 1: Expand nsRange::ExtractRectFromOffset to correctly generate rects from vertical text runs.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1302470
[User impact if declined]: Text in vertical writing modes may not be findable
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Doesn't change code path for finding text in horizontal writing modes, which has been working well
[String changes made/needed]: None

	Julien Cristau [:jcristau] (back April 22)
	Comment 18 • 6 years ago

Comment on attachment 8957342 [details]
Bug 1437509 Part 1: Expand nsRange::ExtractRectFromOffset to correctly generate rects from vertical text runs.

fix find in page for vertical text, approved for 60.0b14

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 19 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/20d2e80aa71b
https://hg.mozilla.org/releases/mozilla-beta/rev/28ef3feed53a