1279843 - preceding and trailing character are obscured by the emphasized find term

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Alice0775 White]

Attachment: screenshot

This is important problem for CJK language. Because word does not separate with white space.

Steps to reproduce:
1. Open https://ja.wikipedia.org/wiki/%E8%BE%BB%E5%B8%8C%E7%BE%8E
2. Ctrl+F
3. Type オーディション

Actual results:
Preceding and trailing character are obscured by the emphasized find term.
So, I cannNot read the statement.

Expected Results:
Should not.

Comment 1

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

[Tracking Requested - why for this release]: Regression

Comment 2

[The 8472]

Attachment: highlight2.png

For completeness sake: This also affects adjacent lines if they have characters descending below their baseline or a reduced line-height

Comment 3

[Marcia Knous [:marcia]]

Feature has been backed out so not tracking these bugs for 50.

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: Design update by Sevaan

Comment 5

[Mike de Boer [:mikedeboer]]

I will deal with the part of the design update that deals with the yellow box spanning multiple lines in bug 1282752.

Comment 6

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1279843 - adjust the borders of the yellow range outline box on the findbar modal highlighting background to have gradient borders ending transparently, so that no characters will be obscured. ui-r=sevaan,

Review commit: https://reviewboard.mozilla.org/r/67944/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67944/

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8775915 [details]
Bug 1279843 - adjust the borders of the yellow range outline box on the findbar modal highlighting background to have gradient borders ending transparently, so that no characters will be obscured. ui-r=sevaan,

https://reviewboard.mozilla.org/r/67944/#review65126

Tentative r+, curious about the RTL question.

::: toolkit/modules/FinderHighlighter.jsm:55
(Diff revision 1)
>  .findbar-modalHighlight-outline[grow] {
> -  transform: scaleX(1.5) scaleY(1.5)
> +  /*transform: scaleX(1.5) scaleY(1.5)*/
>  }

If this isn't necessary anymore please delete this rule.

::: toolkit/modules/FinderHighlighter.jsm:87
(Diff revision 1)
>  .findbar-modalHighlight-rect {
>    background: #fff;
> -  border: 1px solid #666;
> +  margin: 0 0 0 -2px;

Does this need to be flipped for RTL? What about vertical writing modes? What is this trying to accomplish? :)

Comment 8

[The 8472]

partial matches would also be of interest, especially in writing systems that don't always use whitespace or fuse characters.

Comment 9

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> Does this need to be flipped for RTL? What about vertical writing modes?
> What is this trying to accomplish? :)

Another, more complete patch is coming up, but wrt the RTL question: we position the rectangles using CSS top and left properties, so that's why I can correct the margins this way.

Comment 10

[Mike de Boer [:mikedeboer]]

Comment on attachment 8775915 [details]
Bug 1279843 - adjust the borders of the yellow range outline box on the findbar modal highlighting background to have gradient borders ending transparently, so that no characters will be obscured. ui-r=sevaan,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67944/diff/1-2/

Comment 11

[Sevaan Franks [:sevaan]]

(In reply to Mike de Boer [:mikedeboer] from comment #10)
> ui-r?sevaan

I can definitely review, but I'll need a screenshot. Thanks!

Comment 12

[Mike de Boer [:mikedeboer]]

Comment on attachment 8775915 [details]
Bug 1279843 - adjust the borders of the yellow range outline box on the findbar modal highlighting background to have gradient borders ending transparently, so that no characters will be obscured. ui-r=sevaan,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67944/diff/2-3/

Comment 13

[Mike de Boer [:mikedeboer]]

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

Comment 14

[Mike de Boer [:mikedeboer]]

Comment on attachment 8775915 [details]
Bug 1279843 - adjust the borders of the yellow range outline box on the findbar modal highlighting background to have gradient borders ending transparently, so that no characters will be obscured. ui-r=sevaan,

Sevaan, I produced a Nightly build for you to test it a bit. I thought that'd be a more fun way to review this.

Comment 15

[Sevaan Franks [:sevaan]]

(In reply to Mike de Boer [:mikedeboer] from comment #12)
> Comment on attachment 8775915 [details]
> Bug 1279843 - update the design of the modal highlight feature. ui-r?sevaan,

Looking really good and almost there. Couple changes:

- The white box around highlighted search terms should have eual padding around it (see how it's bottom heavy at the moment?)
- The yellow highlight should completely cover the white highlights.

Comment 16

[Mike de Boer [:mikedeboer]]

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

Comment 17

[Mike de Boer [:mikedeboer]]

Comment on attachment 8775915 [details]
Bug 1279843 - adjust the borders of the yellow range outline box on the findbar modal highlighting background to have gradient borders ending transparently, so that no characters will be obscured. ui-r=sevaan,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67944/diff/3-4/

Comment 18

[Mike de Boer [:mikedeboer]]

Comment on attachment 8775915 [details]
Bug 1279843 - adjust the borders of the yellow range outline box on the findbar modal highlighting background to have gradient borders ending transparently, so that no characters will be obscured. ui-r=sevaan,

Sevaan, I made another build for you to check: http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-16bb4bf82b70417d08af6b5dfa3ebb5bc0790620/

Could you review the changes for me? There might be 1px offset difference between different websites, but that's because `getClientRect()` is off-by-one occasionally. I'll need to file a bug for that, but this is FYI. Thanks!

Comment 19

[Mike de Boer [:mikedeboer]]

Oh and please flip the pref 'findbar.highlightAll' and 'findbar.modalHighlight' to 'true' in about:config, like before.

Comment 20

[Sevaan Franks [:sevaan]]

Comment on attachment 8775915 [details]
Bug 1279843 - adjust the borders of the yellow range outline box on the findbar modal highlighting background to have gradient borders ending transparently, so that no characters will be obscured. ui-r=sevaan,

Hi Mike,

Almost there!

For the regular body text, the boxes look good: http://i.sevaan.com/3a0f2H3L3j47

However, when the font size is larger, such as in header tags, the white box around the text does not have the same ratio of margins around it...see how the white highlight should extend down more? http://i.sevaan.com/2y1j1l0L2U02

Comment 21

[Mike de Boer [:mikedeboer]]

(In reply to Sevaan Franks [:sevaan] from comment #20)
> However, when the font size is larger, such as in header tags, the white box
> around the text does not have the same ratio of margins around it...see how
> the white highlight should extend down more? http://i.sevaan.com/2y1j1l0L2U02

I can make it like this: https://www.evernote.com/l/APsO8GjAKbBDzqmzrlErXGdETOUwx99GM0A
What do you think?

Comment 22

[Sevaan Franks [:sevaan]]

(In reply to Mike de Boer [:mikedeboer] from comment #21)
> I can make it like this:
> https://www.evernote.com/l/APsO8GjAKbBDzqmzrlErXGdETOUwx99GM0A
> What do you think?

Looks great. Can you add an extra pixel to the top of the body text highlights as well? See how the header text is evenly padded on the top and the bottom? The body should be like that too.

Comment 23

[Mike de Boer [:mikedeboer]]

(In reply to Sevaan Franks [:sevaan] from comment #22)
> (In reply to Mike de Boer [:mikedeboer] from comment #21)
> Looks great. Can you add an extra pixel to the top of the body text
> highlights as well? See how the header text is evenly padded on the top and
> the bottom? The body should be like that too.

I'm afraid that's what I mean by the last paragraph in comment 18: "There might be 1px offset difference between different websites, but that's because `range.getClientRect()` is off-by-one occasionally. I'll need to file a bug for that, but this is FYI"

Comment 24

[Mike de Boer [:mikedeboer]]

Comment on attachment 8775915 [details]
Bug 1279843 - adjust the borders of the yellow range outline box on the findbar modal highlighting background to have gradient borders ending transparently, so that no characters will be obscured. ui-r=sevaan,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67944/diff/4-5/

Comment 25

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/f76b9d417a3678f840f498598d215b41f5c6616a
Bug 1279843 - update the design of the modal highlight feature. ui-r=sevaan, r=jaws

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f76b9d417a36

Comment 27

[The 8472]

This isn't solved, depending on font-stretch narrow characters like i and l can still get obscured partially or completely by the highlight.

It would be better if the highlight box were positioned behind the text like a background, not over it.

Comment 28

[The 8472]

Attachment: search-overlay.png

The issue as of nightly 20160911030419

Comment 29

[roxleitan]

Attachment: find toolbar.png

Verified on the latest Nightly 52.0a1, build ID: 20160922030437, on Windows 10 x64 and the issue is still reproducible.
See attachment.
Based on this attachment and comment 28 I will reopen the bug.

Comment 30

[Justin Dolske [:Dolske]]

Seems there's some doubt if there's a good UX path forward here -- it's a tradeoff between making the current highlight readable and looking good, vs showing things that are immediately adjacent.

Random idea that came to mind: use a yellow border instead of padding (same color), and make the border semi-transparent?

Comment 31

[The 8472]

that might be an improvement but even that still might hurt legibility.

I assume it's non-trivial due to the way the highlight is implemented, but ideally the yellow highlight should go below the text layer (of the non-highlighted text). In other words, it should be a properly-placed background-image on the containing element of the text.

Comment 32

[Justin Dolske [:Dolske]]

Clearing status flags, this feature is Nightly-only as of bug 1303248, so we don't need to track.

Comment 33

[Mike de Boer [:mikedeboer]]

Sevaan, I can make it look like this: https://www.evernote.com/l/APs34sZt3e1JSI5KZY7rJRW9bOfdaIqflhE which adds a slowly moving to transparent border around the yellow box.
This looks like https://www.evernote.com/l/APuNj1VFAHBJtb3PUrruQxhUgFFXvGyRSzo for adjacent characters. What do you think?

Comment 34

[Mike de Boer [:mikedeboer]]

Attachment: Patch: gradient borders

Comment 35

[Sevaan Franks [:sevaan]]

(In reply to Mike de Boer [:mikedeboer] from comment #33)
> Sevaan, I can make it look like this:
> https://www.evernote.com/l/APs34sZt3e1JSI5KZY7rJRW9bOfdaIqflhE which adds a
> slowly moving to transparent border around the yellow box.
> This looks like
> https://www.evernote.com/l/APuNj1VFAHBJtb3PUrruQxhUgFFXvGyRSzo for adjacent
> characters. What do you think?

Clever idea :Dolske and :mikedeboer. Looks good to me!

Comment 36

[The 8472]

Have you tested that approach with scripts that fuse characters? arabic, thai, brahmic scripts maybe?

Comment 37

[Mike de Boer [:mikedeboer]]

No, because I don't know how TBH - can you give me an example I can test this with? Maybe even a couple?

Comment 38

[The 8472]

I just open some wikipedia pages in the appropriate languages and pick out characters really close to narrow characters and put them in the search box.

e.g. https://pnb.wikipedia.org/wiki/%D8%B9%D8%B1%D8%A8%DB%8C search for ی
or https://sa.wikipedia.org/wiki/%E0%A4%AD%E0%A4%BE%E0%A4%B0%E0%A4%A4%E0%A5%80%E0%A4%AF%E0%A4%B2%E0%A4%BF%E0%A4%AA%E0%A4%AF%E0%A4%83 search for र (the 27th match seems especially interesting)

I think it might be necessary to clone adjacent text too so the font renderer can reproduce the same ligatures as the underlying text and then clip (css mask) instead of only replicating the partial match. That way the match should blend into the background if the font properties are reproduced accurately

Some edge-cases:

Old Mongolian script - http://www.babelstone.co.uk/Test/Mongolian.html
Mixed vertical/horizontal writing - https://developer.mozilla.org/en-US/docs/Web/CSS/text-combine-upright#Example_(all)

Comment 39

[Mike de Boer [:mikedeboer]]

Oooh, you're right here and that's a different bug altogether! I mean, single characters split-off from ligatures are something that I can't support with the current implementation.
So this is a bug, but out of the scope for this particular one.
What I can and should do here is to make sure that the yellow box never outgrows the width & height of the ranges' rectangles, so that at least the original ligature doesn't get obscured in case the split-off character is larger than when it's part of that ligature.
This, with an added overflow: hidden I can even yank out the text including three - may be more - complete unicode codepoints to make sure the ligature is recomposed in the yellow outline. This could work, but might introduce a regression. I'll have to experiment here.

Comment 40

[Mike de Boer [:mikedeboer]]

Comment on attachment 8775915 [details]
Bug 1279843 - adjust the borders of the yellow range outline box on the findbar modal highlighting background to have gradient borders ending transparently, so that no characters will be obscured. ui-r=sevaan,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67944/diff/5-6/

Comment 41

[Mike de Boer [:mikedeboer]]

Comment on attachment 8775915 [details]
Bug 1279843 - adjust the borders of the yellow range outline box on the findbar modal highlighting background to have gradient borders ending transparently, so that no characters will be obscured. ui-r=sevaan,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67944/diff/6-7/

Comment 42

[The 8472]

> including three - may be more - complete unicode codepoints

I'm afraid that you'll have to be even more conservative than that. Some codepoints can have a spooky action at a distance. E.g. the RTL and LTR overrides. There are really a lot of pitfalls in unicode https://en.wikipedia.org/wiki/Complex_text_layout (that page isn't even exhaustive)

Comment 43

[Mike de Boer [:mikedeboer]]

Comment on attachment 8775915 [details]
Bug 1279843 - adjust the borders of the yellow range outline box on the findbar modal highlighting background to have gradient borders ending transparently, so that no characters will be obscured. ui-r=sevaan,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67944/diff/7-8/

Comment 44

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8796153 [details] [diff] [review]
Patch: gradient borders

Review of attachment 8796153 [details] [diff] [review]:
-----------------------------------------------------------------

Please file a bug for comment 38 and comment 39.

::: toolkit/modules/FinderHighlighter.jsm
@@ +42,5 @@
>    outlineNode: [
>      ["position", "absolute"],
> +    ["background-color", `rgb(${kOutlineBoxColor})`],
> +    ["background-clip", "padding-box"],
> +    ["border", `2px solid rgba(${kOutlineBoxColor},.1)`],

The border-color specified here isn't used. Please use long-hand here to remove confusion.

border-width: 2px;
border-style: solid;
-moz-border-top-colors: ...

Comment 45

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/e70b480ce6c24db5f13e1c5f402592d1ef0922ab
Bug 1279843 - adjust the borders of the yellow range outline box on the findbar modal highlighting background to have gradient borders ending transparently, so that no characters will be obscured. ui-r=sevaan, r=jaws

Comment 46

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e70b480ce6c2

Comment 47

[roxleitan]

Verified as fixed using the latest Nightly 52.0a1 (Build ID:20161013030204) on Ubuntu 16.04, Windows 10 and Mac OS X 10.11