Closed Bug 1437509 Opened 2 years ago Closed 2 years ago

"Quick find" and "Quick find (links only)" don't always work for vertical texts

Categories

(Toolkit :: Find Toolbar, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: oana.botisan, Assigned: bradwerth)

References

Details

(Keywords: regression)

Attachments

(2 files)

[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.
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.
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.
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.
Flags: needinfo?(bwerth)
Priority: -- → P2
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.
Assignee: nobody → bwerth
Flags: needinfo?(bwerth)
Attachment #8957342 - Flags: review?(bzbarsky)
Attachment #8957343 - Flags: review?(mdeboer)
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.
Attachment #8957342 - Flags: review?(bzbarsky) → review+
(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.
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...
Attachment #8957343 - Flags: review?(mdeboer) → review+
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
https://hg.mozilla.org/mozilla-central/rev/0a91126bb0f9
https://hg.mozilla.org/mozilla-central/rev/fe9a67a7ff0d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1450208
Blocks: 1279715
Is there a user impact here which justifies uplift consideration or can this ride the 61 train?
Flags: needinfo?(bwerth)
(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.
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
Flags: needinfo?(bwerth)
Attachment #8957342 - Flags: approval-mozilla-beta?
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
Attachment #8957342 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1455742
Duplicate of this bug: 1159309
Duplicate of this bug: 1439891
You need to log in before you can comment on or make changes to this bug.