Closed Bug 407944 Opened 17 years ago Closed 6 years ago

for rich autocomplete performance, correctness (rtl) and features (selecting all matches), use one html:span and use selection

Categories

(Firefox :: Address Bar, defect)

Product:
Firefox
Component:
Address Bar
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED INACTIVE

People

(Reporter: moco, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: access, perf)

Attachments

(5 files)

screenshot of v1
77.00 KB, image/png
Details
v1
8.17 KB, patch
Details | Diff | Splinter Review
v1.1
8.29 KB, patch
Details | Diff | Splinter Review
screenshot of v1 of bug 415403 (att bug moz https)
114.18 KB, image/png
Details
screenshot of v1 of bug 415403 flipped (att bug moz https)
117.27 KB, image/png
Details 
for rich autocomplete performance, correctness (rtl) and features (selecting all matches), use one html:span and use selection

This is vlad's idea:

instead of:

<description>123<label value="456"/>789 123456789</description>

or even:

<description>123<html:span>456<html:span/>789 123456789</description>

(Which we need to do for RTL)

We could do:

<description><html:span>123456789 123456789</html:span></description> and implement emphasis using selection.

In this case, our selection would contain two ranges (one for the first "456" and the second for the second "456").  This would allow us to select all matches, not just the first,

Until we have a style, we should set the selection type to be SELECTION_NORMAL or SELECTION_SPELLCHECK.

Vlad believed this will also improve performance as modifying the selection would be faster than setting textContent of the child nodes of the description element and of the value of label (or soon, the textContent of the html:span)
one concern I have is what using selection might do to screen readers, but we could just continue to set the label on the richlist item to be the complete text, and make emphasis a visual only thing.
How's are you going to address your ellipsization requirements in this framework?
Creating new selection type for your needs would be very little work, if we run into any problems reusing an existing selection type.
I think the selection is disabled for XUL windows, so you may still have a performance penalty if you need to enable it. Rendering lots of bits of selection ranges can also be slow.
 

> Rendering lots of bits of selection ranges can also be slow.

That used to be true but it should not be a problem with the new textframe.

> I think the selection is disabled for XUL windows,

I don't know about this. Exactly what is disabled and how?
Both of those selection types cause a11y to fire events for each change. It will probably confuse screen readers. If you create a new selection type or use a different one from those there shouldn't be a problem.
(In reply to comment #5)
> That used to be true but it should not be a problem with the new textframe.

Excellent! So displaying a spellchecked document with thousands of misspellings is fast?

> > I think the selection is disabled for XUL windows,
> 
> I don't know about this. Exactly what is disabled and how?

I don't know exactly. I've tried to discover this but got lost. It's possible it doesn't matter considering the previous comment. I'd recommend using a new SELECTION_* constant anyway.

(In reply to comment #7)
> (In reply to comment #5)
> > That used to be true but it should not be a problem with the new textframe.
> 
> Excellent! So displaying a spellchecked document with thousands of misspellings
> is fast?

I think so, yes.
> How's are you going to address your ellipsization requirements in this 
> framework?

Until we have bug #312156 (implement text-overflow: ellipsis from CSS3 text), I was planning to do what we currently do, which is to have the description inside a scrollbox, followed by a label.  the value of the label is either the localized ellipsis character or "", depending on if the description element is bigger than the scrollbox, see _setUpEllipsis()

http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/autocomplete.xml#1266

Once we had #312156, or if we had another way of making crop attribute work with description even I don't use the value attribute (see bug #404427), I could remove all those scrollboxes (two per autocomplete item, so 50 in total by default), the additional labels for the ellipsis, and the hacky code to hide/show the ellipsis (which gets called on setTimeout.)
Blocks: 405745
neil/aaron/roc:  vlad also suggested that we need a new SELECTION_* constant, but until we had one, or to verify this approach would work, we could use an existing SELECTION_* constant locally.

As far as selection not working in XUL, in thunderbird, I seem to be able to select the subject text in the message header pane, so it must be possible, right?
Component: Places → Location Bar and Autocomplete
QA Contact: places → location.bar
Some kinds of selection work, just not using the general framework (and I don't think multiple ranges work).

Note that you really, really, really want to get rid of the scrollboxes for the final, regardless if things get implemented using selection or something else.  They cause a native OS widget to be created per scrollbox.
> Note that you really, really, really want to get rid of the scrollboxes

I've logged that issue as bug #407984, and I think it is doable with what we have now following the trick you showed me last week.
Any progress/thoughts about the new selection type? This would be useful for showing all matches of a word in the title/url as well as showing all matches of each search term like for bug 401869.
Typing in the new URL bar is extremely slow when a11y is active, because a11y listens for any DOM changes and does a lot of internal work with them.

If there is any way to reduce the number of mutations it would help a lot.
Blocks: fox3access
Keywords: access, perf
I'm not sure who's responsible for what here. Is someone (me?) supposed to be implementing a new selection type? That's easy. I assume you want a new CSS pseudo-class :-moz-selected-aux or something like that.
Am I on the right track here?

Add a new selection type to nsISelectionController:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/base/public/nsISelectionController.idl&rev=3.23&mark=57-65#56

Search for word matches and create nsIDomRange for each match and set start/end:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/dom/public/idl/range/nsIDOMRange.idl&rev=1.6&mark=72-75#72

Add each nsIDOMRange to a nsISelection:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/base/public/nsISelection.idl&rev=1.11&mark=138-141#138

And somehow style those selections differently?
(In reply to comment #15)
> Is someone (me?) supposed to be implementing a new selection type?
That should just be adding a new value to nsISelectionController.idl, right?

> I assume you want a new CSS
> pseudo-class :-moz-selected-aux or something like that.
How would I go about doing that?

I looking into how typeaheadfind works and it uses a nsIFind to get the nsIDOMRanges, so I should be able to get ranges to add to a selection for a new selection type. What should I look for to see how to add a new css pseudo-class?
That's all the right idea, yeah!
(In reply to comment #16)
> Am I on the right track here?
> 

Yes, you would need to add a new nsISelectionController constant and for styling you probably want to use something similar to how -moz-selection works.

Actually, thinking about it, you might be able to just use the regular selection and use and/or expand -moz-selection to support whatever highlighting was desired.

.urlbar-highlight::-moz-selection {
  background: lightgreen;
}
that might work, as long as you don't need any other selections in the browser chrome. Edit controls like the URL bar have their own selections so they wouldn't be a problem.
Attached image screenshot of v1 — 
I'm getting strange highlighting behavior.. The selection contains the exact term being searched, but what gets styled is much more than that.. sometimes.

(I'm just using the plain selection with window.getSelection().addRange())

I've got some debugging text to show what the selection thinks it has.. and notice it only has the words "planet":

Adding range 'Planet' for Planet Mozilla Blog. Full selection: PlanetplanetPlanetplanetPlanetplanetplanetPlanetplanetplanet
Attached patch v1 — — Splinter Review 
Initial stab at just selecting stuff using the existing normal selection.
I'm not sure why sometimes it selects nothing, the correct part, the whole thing.

Sometimes it works.. sometimes not. Also, there's issues of the highlighted sections flashing as results come in... But interesting thing is for the ones that end up selecting nothing, they *do* sometimes flash the expected highlighted area.
Blocks: 415403
Attached patch v1.1 — — Splinter Review 
Not much different other than saving bitrot for bug 415403.
Attachment #301070 - Attachment is obsolete: true
Comment on attachment 301070 [details] [diff] [review]
v1

>-# note, we rely on the newlines here so that we have
>-# a textNode before and after the span.  see _setUpDescription()
>-# for more details
>           <xul:description anonid="title" class="ac-normal-text ac-comment" xbl:inherits="selected">
>-            <html:span class="ac-emphasize-text"/>
>+            # we're using this space to have a textNode

If you want to talk to the preprocessor, start at the beginning of the line.

>+.ac-normal-text::-moz-selection {
>+  background: lightgreen;
> }

Should use rgba() here or specify a foreground color.
Attachment #301070 - Attachment is obsolete: false
(In reply to comment #26)
> >+            # we're using this space to have a textNode
> If you want to talk to the preprocessor, start at the beginning of the line.
Whoops. Thanks.

> >+.ac-normal-text::-moz-selection {
> >+  background: lightgreen;
> Should use rgba() here or specify a foreground color.
You mean rgba instead of lightgreen? That color is probably just temporary, but rgba for the future. Ok. Did you mean we should additionally set the foreground color?

roc: So ::-moz-selection is supposed to only allow background and color? No setting of font-weight or text-decoration? Would we be able to get that functionality? (I suppose CSS3 ::selection might only allow the color changes..)

Also, I'm getting a bunch of these NS_NOTREACHED sometimes for nsLineLayout::BeginLineReflow.

###!!! ASSERTION: bad width: 'Not Reached', file layout/generic/nsLineLayout.cpp, line 179
Area(description)(0)@0x27855c4: Init: bad caller: width WAS 879290(0xd6aba)
nsLineLayout: Text(0)@0x2785358 metrics=879290,900!

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsLineLayout.cpp&rev=3.296&mark=169,178-183#169

I've got some builds here that has the multi-word highlighting:
https://build.mozilla.org/tryserver-builds/2008-02-02_21:53-edward.lee@engineering.uiuc.edu-matchall.adaptive/

If people want to see the incorrect higlighting first hand. Check the error console to see what the selection thinks it has highlighted (the correct words), but the UI shows otherwise.
Now that's odd. If I flip these two lines, the highlighting changes quite a bit.

>+          this._setUpDescription(this._title, title, this._titleBox.boxObject, this._titleOverflowEllipsis); 
>+          this._setUpDescription(this._url, url, this._urlBox.boxObject, this._urlOverflowEllipsis);

So flipped would highlight the url first then title second for each entry.

Looking at the screenshot, the title actually gets "Att" and "Bug" highlighted correctly whereas before in attachment 301083 [details], they were never highlighted unless the whole title got highlighted.
(In reply to comment #27)
> > >+.ac-normal-text::-moz-selection {
> > >+  background: lightgreen;
> > Should use rgba() here or specify a foreground color.
> You mean rgba instead of lightgreen? That color is probably just temporary, but
> rgba for the future. Ok. Did you mean we should additionally set the foreground
> color?

Using rgba() to colorize the parent background should be fine. Only if you're specifying an opaque background (especially a non-native one), a corresponding foreground color needs to be specified as well. Otherwise you get bugs like bug 409974 (in that case, we have a hardcoded foreground color without a background color).
Am I doing anything wrong to get results like those in attachment 301083 [details] or attachment 301099 [details]?
Comment on attachment 301075 [details] [diff] [review]
v1.1

>Bug 407944 - for rich autocomplete performance, correctness (rtl) and features (selecting all matches), use one html:span and use selection
You're correctly not using html:spam so perhaps update the title ;-)
faaborg, beltzner: Would it be okay to only style the highlighted text by changing the font color and/or background color (instead of bold and underline)? See attachment 301083 [details] (ignore the issue of it highlighting the whole line). This would fix a couple of bugs like bug 407861 and bug 406254

The CSS3 spec for ::selection only allows a certain subset of styles:

"These are the CSS properties that apply to ::selection pseudo-elements: color, background, cursor (optional), outline (optional). The computed value of the 'background-image' property on ::selection may be ignored."

roc: Any estimate on how much work would be needed (perhaps not by me ;)) to either add support for font-weight or text-decoration to ::-moz-selection or a completely separate selection type? (I probably won't be able to just use window.getSelection() because that uses the default selection.)

(In reply to comment #31)
> You're correctly not using html:spam so perhaps update the title ;-)
Well, the patch is nothing final yet, so I'll leave the title be for now.
Depends on: 415860
font-weight is probably a lot of work -- note that all the current properties (color, background, outline) don't require reflow or a new layout of text, since the individual glyphs don't move.  Changing the font-weight would.  However, text-decoration shouldn't, so that should be straightforward.

Makes sense about reflow. For a moment I thought outline had outline-bottom like borders, but apparently not. But you can use a negative offset to get something like an underline. ;)

data:text/html,<span style="outline: 1px inset #000; outline-offset: -.9em;">Hello World!</span>
text-decoration would be easy. We already support decoration-like effects for some of the special selections, e.g. spellcheck and IME.

Let me know if you definitely need this...
(In reply to comment #35)
> Let me know if you definitely need this...
Still seeing if we should go down this path of using selections and all:

Even with the current patch for bug 415860 applied or assigning textContent = "" before assigning the real value, those don't help with some of the issues of the rendering showing the whole line as selected. So there's possibly another bug.

I'm seeing a bunch of those not-reached assertions about huge widths. That might also be related to the issue of the selections flickering when results are being added.

Lesser issues that are probably artifacts of the current patch include the selections going away if the user focuses the popup and leaves it (back to the location bar). Also, because I've set -moz-user-select: -moz-all, the user can select text in the dropdown with the cursor.
>faaborg, beltzner: Would it be okay to only style the highlighted text by
>changing the font color and/or background color (instead of bold and
>underline)?

Our main objective is to make it clear to the user what section matched against their search, while still maintaing the readability of the highlighted information and the surrounding information.  So far I think the combination of bold and underline has worked pretty well visually, but it isn't necessarily the only approach.  For instance, I like this styling for OS X:

https://bugzilla.mozilla.org/attachment.cgi?id=290219




(In reply to comment #32)
> faaborg, beltzner: Would it be okay to only style the highlighted text by
> changing the font color and/or background color (instead of bold and
> underline)? [...]

IIRC, highlights consisting _only_ of colour changes should be avoided because of accessibility problems. I don't remember the full details though, and I'm no accessibility specialist. Maybe someone else remembers the policy better than I do?
>highlights consisting _only_ of colour changes should be avoided because
>of accessibility problems.

A combination of certain shades of colors can be impossible to distinguish by users who have different types of color blindness.  However, color changes that involve contrast, like a darker shade of green next to a lighter shade of green, will be noticeable by all users, it just won't necessarily look green.
Flags: wanted-firefox3+
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
Not sure how, but the RTL issue was fixed a long time ago, so removing rtl keyword.
Keywords: rtl
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: