Closed Bug 1279843 Opened 4 years ago Closed 4 years ago

preceding and trailing character are obscured by the emphasized find term

Categories

(Toolkit :: Find Toolbar, defect)

50 Branch
defect
Not set
major
Points:
3

Tracking

()

VERIFIED FIXED
mozilla51
Iteration:
50.4 - Aug 1
Tracking Status
firefox52 --- verified

People

(Reporter: alice0775, Assigned: mikedeboer)

References

(Blocks 2 open bugs)

Details

(Keywords: jp-critical, regression)

Attachments

(7 files)

Attached image 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.
Attached image highlight2.png
For completeness sake: This also affects adjacent lines if they have characters descending below their baseline or a reduced line-height
Feature has been backed out so not tracking these bugs for 50.
Attached image Design update by Sevaan
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 50.4 - Aug 1
Points: --- → 3
Flags: qe-verify+
Flags: firefox-backlog+
I will deal with the part of the design update that deals with the yellow box spanning multiple lines in bug 1282752.
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? :)
Attachment #8775915 - Flags: review?(jaws) → review+
partial matches would also be of interest, especially in writing systems that don't always use whitespace or fuse characters.
(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 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/
(In reply to Mike de Boer [:mikedeboer] from comment #10)
> ui-r?sevaan

I can definitely review, but I'll need a screenshot. Thanks!
Blocks: 1291278
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 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.
Attachment #8775915 - Flags: ui-review?(sfranks)
(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.
Attachment #8775915 - Flags: ui-review?(sfranks) → ui-review-
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 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/mdeboer@mozilla.com-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!
Attachment #8775915 - Flags: ui-review- → ui-review?(sfranks)
Oh and please flip the pref 'findbar.highlightAll' and 'findbar.modalHighlight' to 'true' in about:config, like before.
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
Attachment #8775915 - Flags: ui-review?(sfranks) → ui-review-
(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?
Flags: needinfo?(sfranks)
(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.
Flags: needinfo?(sfranks)
(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"
Flags: needinfo?(sfranks)
https://hg.mozilla.org/integration/fx-team/rev/f76b9d417a3678f840f498598d215b41f5c6616a
Bug 1279843 - update the design of the modal highlight feature. ui-r=sevaan, r=jaws
Flags: needinfo?(sfranks)
Attachment #8775915 - Flags: ui-review- → ui-review+
https://hg.mozilla.org/mozilla-central/rev/f76b9d417a36
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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.
Attached image search-overlay.png
The issue as of nightly 20160911030419
Attached image 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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
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.
Clearing status flags, this feature is Nightly-only as of bug 1303248, so we don't need to track.
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?
Flags: needinfo?(sfranks)
(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!
Flags: needinfo?(sfranks)
Attachment #8796153 - Flags: review?(jaws)
Have you tested that approach with scripts that fuse characters? arabic, thai, brahmic scripts maybe?
No, because I don't know how TBH - can you give me an example I can test this with? Maybe even a couple?
Flags: needinfo?(bugzilla.mozilla.org)
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)
Flags: needinfo?(bugzilla.mozilla.org)
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.
Blocks: 1305033
> 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 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: ...
Attachment #8796153 - Flags: review?(jaws) → review+
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
https://hg.mozilla.org/mozilla-central/rev/e70b480ce6c2
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Blocks: 1309155
Verified as fixed using the latest Nightly 52.0a1 (Build ID:20161013030204) on Ubuntu 16.04, Windows 10 and Mac OS X 10.11
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.