Closed Bug 1446830 Opened 6 years ago Closed 6 years ago

Don't use a XUL label for displaying the selected filename in the filepicker UI

Categories

(Core :: XUL, task, P5)

task

Tracking

()

RESOLVED FIXED

People

(Reporter: bgrins, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [overhead:noted])

Attachments

(4 files, 3 obsolete files)

Using a <xul:label> [0] means that the label XBL binding runs in-content, and that we have XUL text CSS selectors (like label) inside of minimal-xul.css - although I believe the CSS could move into xul.css ahead of changing the implementation here, given XULElementsRulesInMinimalXULSheet [1].  There are also some CSS rules targeting `input[type="file"] > xul|label` in forms.css [2].

[0] https://searchfox.org/mozilla-central/rev/3abf6fa7e2a6d9a7bfb88796141b0f012e68c2db/layout/forms/nsFileControlFrame.cpp#144
[1] https://searchfox.org/mozilla-central/rev/3abf6fa7e2a6d9a7bfb88796141b0f012e68c2db/dom/xul/nsXULElement.cpp#683
[3] https://searchfox.org/mozilla-central/rev/3abf6fa7e2a6d9a7bfb88796141b0f012e68c2db/layout/style/res/forms.css#504
Blocks: 1446831
(In reply to Brian Grinstead [:bgrins] from comment #0)
> we have XUL text CSS selectors (like label) inside of
> minimal-xul.css - although I believe the CSS could move into xul.css ahead
> of changing the implementation here, given XULElementsRulesInMinimalXULSheet
> [1].

Correction: we *will* need to change the implementation here before moving these out of minimal-xul.css (Bug 1446831), since we won't ever be loading xul.css in the content document after Bug 1444193.
Comment on attachment 8960044 [details]
Bug 1446830 - Alternate implementation that uses HTML label with end crop instead of center

https://reviewboard.mozilla.org/r/228808/#review234502

::: layout/forms/nsFileControlFrame.cpp:476
(Diff revision 1)
>  #endif
>  
>  void
>  nsFileControlFrame::UpdateDisplayedValue(const nsAString& aValue, bool aNotify)
>  {
> -  mTextContent->SetAttr(kNameSpaceID_None, nsGkAtoms::value, aValue, aNotify);
> +  // XXX: XUL label allows crop: center. Can we use CSS to get similar behavior?

Nice find.

for <xul:label crop=center> can we could use CSS text-overflow to do create similar visuals?
Attached file file-input-sizes.html
A test document with widths set on both the file input and the parent div of the file inputs. The only case I can trigger the crop=center behavior is when the width is set on the file input and it's not too small (I start to see the ellipses around 175px).
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #4)
> for <xul:label crop=center> can we could use CSS text-overflow to do create
> similar visuals?

Unfortunately middle cropping isn't part of the standard (see also bug 740910). We can probably make cropping at the end work with CSS, although in that case you wouldn't see the file extension.
See Also: → 740910
(In reply to Brian Grinstead [:bgrins] from comment #6)
> (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment
> #4)
> > for <xul:label crop=center> can we could use CSS text-overflow to do create
> > similar visuals?
> 
> Unfortunately middle cropping isn't part of the standard (see also bug
> 740910). We can probably make cropping at the end work with CSS, although in
> that case you wouldn't see the file extension.

Neil, do you have any ideas on how we could accomplish a crop="middle" effect for an HTML label inside the file picker's anon content? Right now I'm just setting text content for the label (see Part 1) but I guess we could render out the DOM however we wanted.
Flags: needinfo?(enndeakin)
Oh also: crop=center in general seems pretty lightly used https://searchfox.org/mozilla-central/search?q=crop%3D%22center%22&path=. Maybe we should come up with a way to do 'filename cropping' for this case and for downloads instead of supporting generic center cropping behavior on xul / html labels
FWIW: On the test page with a long filename (really-long-file-name-123456789-abcdefghijklmnopqrstuvwxyz.txt) I see some odd behavior where the center crop doesn't show the end of the string so you can't see the file extension.
Blocks: war-on-xbl
There isn't any other text displaying frame that crops like that.
Flags: needinfo?(enndeakin)
Suggestion from roc: maybe you could split the filename into two span elements, one containing the directory and one the file name itself, and make the directory element text-overflow:ellipsis, then put both elements in a flexbox so that the filename gets its intrinsic width and the directory element gets whatever is left over. Include a min-width on the directory part so it won't collapse down to 0.
Suggestion from dholbert: you could build a chrome-only "text-overflow: middle" (and could begin by prototyping the behavior you want in text-overflow: ellipsis).
See Also: → 1421382
Found a couple of potential CSS solutions for the center crop behavior by splitting the text into two siblings: https://codepen.io/anon/pen/LdWLem and https://codepen.io/anon/pen/ZxeyZJ. Going to spend some more time looking into it.
(In reply to Brian Grinstead [:bgrins] from comment #13)
> Found a couple of potential CSS solutions for the center crop behavior by
> splitting the text into two siblings: https://codepen.io/anon/pen/LdWLem and
> https://codepen.io/anon/pen/ZxeyZJ. Going to spend some more time looking
> into it.

They look ... hacky. I would worry the accessiblity tree these markup results.
Attachment #8960045 - Attachment is obsolete: true
Ehsan, we discussed previously about how the current file picker form control forces ltr on the label that displays the filename. Would you be able to check this iteration of the patch and let me know how it compares with the status quo?

For whatever reason, when I set [dir=auto] on the NAC label it doesn't change direction based on the filename text, so instead this just inherits directionality from the input. Which means if we have an rtl input with ltr filename text it renders with the wrong directionality (basically like what happens always today with rtl filename). I'm not sure if this is better or worse than current behavior.

I set up a test page at https://garnet-wine.glitch.me/ that shows a few different configurations and makes it easier to trigger overflows.
Flags: needinfo?(ehsan)
Sure, any chance you could make a try build please?  Thanks!
(In reply to :Ehsan Akhgari from comment #21)
> Sure, any chance you could make a try build please?  Thanks!

Yes, here it is: https://treeherder.mozilla.org/#/jobs?repo=try&revision=df3b5be6ef96ac52e011eb06abb7d3a9635e6ee9
I think this is worse than the current behavior.  :-(  Note that the text on the button comes from the UI language, so this would mean the "Browse..." text would look broken on all LTR builds when viewing an RTL page.

If you can file a bug about [dir=auto] not working on NAC, I think we can make it work.  I suspect there is something broken in the code in DirectionalityUtils.cpp that doesn't account for native anonymous content properly somehow...  It doesn't seem worth sacrificing a user-facing behavior due to a bug!
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari from comment #23)
> I think this is worse than the current behavior.  :-(  Note that the text on
> the button comes from the UI language, so this would mean the "Browse..."
> text would look broken on all LTR builds when viewing an RTL page.
> 
> If you can file a bug about [dir=auto] not working on NAC, I think we can
> make it work.  I suspect there is something broken in the code in
> DirectionalityUtils.cpp that doesn't account for native anonymous content
> properly somehow...  It doesn't seem worth sacrificing a user-facing
> behavior due to a bug!

OK, I'll file a bug for it. If fixing it turns out to not be viable we could always restore the forced ltr behavior we currently use but ofc it'd be nicer to fix this up while we're here.
Another workaround could also be dropping the dots at the end of these strings, since doing that would mask the directionality of the strings.  ;-)  But if you decide to go down that path please reach out to the l10n folks and consult with them, since ensuring these types of hacks work properly across 80+ localizations is difficult...
Depends on: 1451576
The use of *nsIContent->SetText() need to be changed to *nsIContent->AsText()->SetText(). The method is removed in bug 1449404.
Priority: -- → P5
Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336
Component: XP Toolkit/Widgets: XUL → XUL
Blocks: 1457628
Whiteboard: [overhead:noted]
Attached image file-input-compare.png
I've pushed up a simpler version that replaces the xul:label with an html:label and a bit of flexbox to get text-overflow to work. The main difference is that instead of cropping the string in the middle (foo…baz.txt) it crops at the end (foobar…).

I think it's possible to replicate the middle crop somehow but it's quite a bit of work - middle-crop isn't a thing in CSS/HTML so we'd need to implement something specific for this use-case.

Given that this is blocking some XBL removal progress and a small content process memory win, I'd like to at least check to see if this is something that we'd be OK with landing as-is even though we'd lose the middle-crop (which lets you see both the beginning and end of a file name). Shorlander, would you be able to give an opinion or redirect this?

A few notes:
1) I've put together a test page at https://garnet-wine.glitch.me/ to see the input in various configurations. The attached screenshot shows what it looks like to select `really-long-file-name-123456789-abcdefghijklmnopqrstuvwxyz.txt` with a constrained width both with and without the patch applied.
2) The current XUL middle crop doesn't always work very well - you can see in the screenshot that you don't see the actual end of the string.
3) The full URL can be seen on hover with the tooltip text in both cases
4) The crop only happens when there's not enough space (usually when a width is set). On cases like bugzilla's file input or `data:text/html,<input type="file">` you can have a pretty huge file name before it starts to crop
Flags: needinfo?(shorlander)
Attached image file-input-sizer.gif
Another example of the current crop not working well. When resizing we first begin clipping the end of the string (with no ellipsis), then eventually a middle crop begins, but the end still isn't visible. A little hard to explain by text so here's a gif.
Attachment #8960044 - Attachment is obsolete: true
Discussed with Mats about middle cropping the text in the label here:

> Implementing middle cropping for arbitrary content isn't easy.
> Bug 740910 has some ideas but none that are fully feasible, IIRC.

> Implementing something specifically for a single text node might
> be easier, especially if it's NAC (no selection or DOM access to
> worry about).
> Then you can just measure the width of the text, chop off characters
> in the middle if it's too wide, and repeat.  We have code to measure
> the width of a string without having to Reflow a frame for it.
> Given that we already have nsFileControlFrame for this, it seems it
> should be possible to override Reflow there and implement something
> like that.
I had an alternate idea which is to implement more of the base <label> behavior in C++ instead of XBL, such that we wouldn't require running JS in the content process for this case. Since we don't need accessKey formatting or handling here maybe we could remove the nsIDOMXULDescriptionElement and nsIDOMXULLabelElement interfaces and instead move the accessible behavior directly into `description`/`label` tag name / attr lookups. Then we could support having a bare <label>, and then only require XBL/Custom Element for more complex cases (label-control and text-link). I think we may need to subclass XULElement in that case so the properties would get attached to the nodes.
Depends on: 1487313
Depends on: 1488116
(In reply to Brian Grinstead [:bgrins] from comment #34)
> I had an alternate idea which is to implement more of the base <label>
> behavior in C++ instead of XBL, such that we wouldn’t require running JS in
> the content process for this case. Since we don’t need accessKey formatting
> or handling here maybe we could remove the nsIDOMXULDescriptionElement and
> nsIDOMXULLabelElement interfaces and instead move the accessible behavior
> directly into `description`/`label` tag name / attr lookups. Then we could
> support having a bare <label>, and then only require XBL/Custom Element for
> more complex cases (label-control and text-link). I think we may need to
> subclass XULElement in that case so the properties would get attached to the
> nodes.

It certainly wouldn’t be without precedent.

(see bug 1437638 and bug 1446961)
Depends on: 1488620
when this bug is fixed, make sure to update minimal-xul.css to move :root text-rendering styles to xul.css, see bug 1446831 comment #11.
Depends on: 1495153
This was fixed by Bug 1495153
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(shorlander)
Resolution: --- → FIXED
Attachment #9005057 - Attachment is obsolete: true
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.