1495153 - Replace the XUL label in <input type=file> with an HTML label

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, enhancement, P4)

Description

[Mats Palmgren (inactive)]

So I hacked up a WIP for this...

Comment 1

[Mats Palmgren (inactive)]

Attachment: part 1 - this replaces the xul:label with a html:span

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 2 - this crops the displayed filename in the middle so that it doesn't overflow

Comment 3

[Mats Palmgren (inactive)]

The html:span seems to confuse some a11y code though:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=02f7621462a150ea13425fa27ccb140c0ad0f6b9&selectedJob=202334110

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Mats Palmgren (:mats) from comment #3)
> The html:span seems to confuse some a11y code though:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=02f7621462a150ea13425fa27ccb140c0ad0f6b9&selectedJob=2
> 02334110

That may be fixable with the correct aria attributes. Alex, any ideas?

Comment 5

[Brian Grinstead [:bgrins]]

As mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1446830#c36, I expect we can also move these text-related CSS rules out of minimal-xul.css and into xul.css once we stop using this <label>: https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/toolkit/content/minimal-xul.css#30-33.

Comment 6

[Mats Palmgren (inactive)]

Attachment: part 1 - this replaces the xul:label with a html:label

Using a html:label instead fixed the a11y failure.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> (In reply to Mats Palmgren (:mats) from comment #3)
> > The html:span seems to confuse some a11y code though:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=02f7621462a150ea13425fa27ccb140c0ad0f6b9&selectedJob=2
> > 02334110
> 
> That may be fixable with the correct aria attributes. Alex, any ideas?

html:label as Mats already did should work.

Comment 8

[Mats Palmgren (inactive)]

BTW, our current filename cropping is horribly buggy.  Try for example:
1. create the file /tmp/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaB
2. load data:text/html,<input type=file style="width:50%; border:1px solid">
3. select that file
Notice that we crop the name but you never see the end of the filename.
It appears we don't account for the width of the Browse button at all. Doh.

Comment 9

[Mats Palmgren (inactive)]

Attachment: part 1 - Replace the XUL label in <input type=file> with an HTML label

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 9013392 [details] [diff] [review]
part 1 - Replace the XUL label in <input type=file> with an HTML label

Jonathan, is "unicode-bidi: bidi-override" the right thing to use here?
When switching from XUL label to HTML label the period in "No file selected."
ended up on the wrong side for dir=rtl:
https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/VbpoNni9T9G8XKHAzbM2eg/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

Comment 11

[Mats Palmgren (inactive)]

Attachment: part 2 - Implement cropping the filename for <input type=file>

The cropping code was copied from the CenterCrop code here:
https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/layout/xul/nsTextBoxFrame.cpp#785-851
so it's still a bit clunky, but good enough for now
I think.

This also fixes the bug mentioned in comment 8.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9013392 [details] [diff] [review]
part 1 - Replace the XUL label in <input type=file> with an HTML label

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

r=me with a reason / comment change for the !important removal (or that change reverted), and Jonathan double-checking the unicode-bidi stuff :)

::: layout/forms/nsFileControlFrame.cpp
@@ +473,5 @@
>  
>  void
>  nsFileControlFrame::UpdateDisplayedValue(const nsAString& aValue, bool aNotify)
>  {
> +  auto* text = mTextContent->GetFirstChild()->AsText();

uber-nit: Maybe Text::FromNode(mTextContent->GetFirstChild()) may make the auto more obvious.

::: layout/style/res/forms.css
@@ -7,5 @@
>   **/
>  
>  
>  @namespace url(http://www.w3.org/1999/xhtml); /* set default namespace to HTML */
> -@namespace xul url(http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul);

Oh wow this is nice actually :)

@@ +506,5 @@
>    /*
>     * Force the text to have LTR directionality. Otherwise filenames containing
>     * RTL characters will be reordered with chaotic results.
>     */
> +  direction: ltr;

Why don't you want to keep the !important? If it's related to your next patch, could the comment and the rule be updated as part of that instead? If not, should the comment above change?

Comment 13

[Mats Palmgren (inactive)]

> Why don't you want to keep the !important?

I figured it was never needed because the label is anonymous content.
There's no way web content can style that element anyway, right?

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9013392 [details] [diff] [review]
part 1 - Replace the XUL label in <input type=file> with an HTML label

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

::: layout/style/res/forms.css
@@ +506,5 @@
>    /*
>     * Force the text to have LTR directionality. Otherwise filenames containing
>     * RTL characters will be reordered with chaotic results.
>     */
> +  direction: ltr;

Err, I guess the label doesn't really match any user rules so this is fine, never mind me.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

(In reply to Mats Palmgren (:mats) from comment #13)
> > Why don't you want to keep the !important?
> 
> I figured it was never needed because the label is anonymous content.
> There's no way web content can style that element anyway, right?

Yes, whoops, I just replied above before seeing this.

Comment 16

[Mats Palmgren (inactive)]

The comment seems accurate also without the !important.  The intent is
to override the inherited value so that <input type=file dir=rtl> still
displays the filename LTR.

Comment 17

[Jonathan Kew [:jfkthame]]

(In reply to Mats Palmgren (:mats) from comment #10)
> Comment on attachment 9013392 [details] [diff] [review]
> part 1 - Replace the XUL label in <input type=file> with an HTML label
> 
> Jonathan, is "unicode-bidi: bidi-override" the right thing to use here?

That doesn't seem right; it will mean that any attempt to use actual CIBARA or WERBEH filenames will render SDRAWKCAB, which is pretty user-hostile.

I think unicode-bidi:isolate may be what you want here, though testing of various combinations of UI direction and filename direction would be useful.

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9013394 [details] [diff] [review]
part 2 - Implement cropping the filename for <input type=file>

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

Looks generally good, though I want to test it a bit tomorrow morning. Some questions below :)

::: layout/forms/nsFileControlFrame.cpp
@@ +69,5 @@
> +    nsLayoutUtils::GetFontMetricsForFrame(this, 1.0f);
> +
> +  // see if the text will completely fit in the width given
> +  nscoord textWidth =
> +    nsLayoutUtils::AppUnitWidthOfStringBidi(aText, this, *fm,

Does this really want `this` as a frame instead of, let's say, the text or the label frame?

@@ +87,5 @@
> +    return;
> +  }
> +
> +  // determine how much of the string will fit in the max width
> +  nscoord totalWidth = textWidth;

This might want to get a comment, saying that we start from the ellipsis, and add from there. Otherwise someone might think that you wanted to actually use the original text width.

::: layout/forms/nsFileControlFrame.h
@@ +159,5 @@
>    /**
> +   * Crop aText to fit inside aWidth.
> +   */
> +  void CropTextToWidth(gfxContext& aRenderingContext,
> +                       nscoord     aWidth,

nit: Let's remove the white-space here given you have not added it to Reflow()?

Or add it there as well (though I don't personally love it :P)

Comment 19

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #17)
> I think unicode-bidi:isolate may be what you want here, though testing of
> various combinations of UI direction and filename direction would be useful.

Or possibly unicode-bidi:plaintext, and drop the direction property (let it be resolved based on the content of the label).

Comment 20

[Mats Palmgren (inactive)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #18)
> > +  nscoord textWidth =
> > +    nsLayoutUtils::AppUnitWidthOfStringBidi(aText, this, *fm,
> 
> Does this really want `this` as a frame instead of, let's say, the text or
> the label frame?

Hmm, yeah that's a good point, AppUnitWidthOfStringBidi calls
nsBidiPresUtils::BidiLevelFromStyle(aFrame->Style()) which is
probably not what we want...

Comment 21

[Mats Palmgren (inactive)]

(In reply to Jonathan Kew (:jfkthame) from comment #19)
> Or possibly unicode-bidi:plaintext, and drop the direction property (let it
> be resolved based on the content of the label).

That seems reasonable to me, but the comment there says that resolving
it based on the filename is undesirable.  Am I misunderstanding what
the comment says?

Comment 22

[Mats Palmgren (inactive)]

Attachment: part 1 - Replace the XUL label in <input type=file> with an HTML label

Removed 'direction' and added 'unicode-bidi: plaintext'.
It places the period correctly in "No file selected." for
<input type=file dir=rtl> at least.

Comment 23

[Mats Palmgren (inactive)]

Attachment: part 2 - Implement cropping the filename for <input type=file>

Made the CropTextToWidth method static and added an aFrame param.

Comment 24

[Mats Palmgren (inactive)]

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

Comment 25

[Mats Palmgren (inactive)]

BTW, the comment in forms.css comes from bug 762270.

Comment 26

[Mats Palmgren (inactive)]

Comment on attachment 9013418 [details] [diff] [review]
part 1 - Replace the XUL label in <input type=file> with an HTML label

Removing 'direction' seems to break <input type=file dir=rtl>,
so I'll need to test this a bit more...

Comment 27

[Mats Palmgren (inactive)]

Attachment: part 1 - Replace the XUL label in <input type=file> with an HTML label

A few changes:
> -  white-space: nowrap;
> +  white-space: nowrap !important;
because we really don't want the label to fragment.
Apparently this wasn't necessary before since XUL doesn't support
continuations so it couldn't occur, but with my changes to
the next part the test layout/forms/crashtests/404118-1.html
started crashing without this.

> -  direction: ltr !important;
> +  -moz-user-select: none;
> +  unicode-bidi: plaintext;

Per Jonathan's advice above.  The user-select:none is to
preserve the behavior that you can't select the filename
(which we got for free with a XUL label).

> -input[type="file"]:dir(rtl) > xul|label {

This rule is wrong now after the 'direction' declaration
was removed.  We automatically get the right layout from
the 'padding-inline-start: 5px' now.

Comment 28

[Mats Palmgren (inactive)]

Attachment: part 2 - Implement cropping the filename for <input type=file>

So the issue with the last patch is that I didn't notify
the text frame about the content change properly.
(The important change here is in UpdateDisplayedValue.)
We can't use aNotify=true during Reflow, so I'm calling
CharacterDataChanged directly instead.  It's a bit hacky
but should work.  The alternative is to recreate the text
frame entirely. (I wrote a patch for that too - I'll
attach it so you can compare it.)

Comment 29

[Mats Palmgren (inactive)]

Attachment: Alternative solution: recreate the text frame every time we change the filename

Comment 30

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9013955 [details] [diff] [review]
part 1 - Replace the XUL label in <input type=file> with an HTML label

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

::: layout/forms/nsFileControlFrame.cpp
@@ +141,5 @@
>    }
>  
>    // Create and setup the text showing the selected files.
> +  mTextContent = doc->CreateHTMLElement(nsGkAtoms::label);
> +  if (!mTextContent) {

CreateHTMLElement doesn't really seem fallible:

https://searchfox.org/mozilla-central/rev/a11c128b90ea85d7bb68f00c5de52c0794e0b215/dom/base/nsDocument.cpp#12531

So I'd remove this null-check.

@@ +155,5 @@
>    nsAutoString value;
>    HTMLInputElement::FromNode(mContent)->GetDisplayFileName(value);
>    UpdateDisplayedValue(value, false);
>  
>    if (!aElements.AppendElement(mTextContent)) {

Unrelated, but this AppendElement is infallible, so if you want to remove the error-checking here as well it'd be great.

Comment 31

[Mats Palmgren (inactive)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #30)
> CreateHTMLElement doesn't really seem fallible:

It calls NS_NewHTMLElement, which calls nsContentUtils::NewXULOrHTMLElement
which appears to be fallible:
https://searchfox.org/mozilla-central/rev/a11c128b90ea85d7bb68f00c5de52c0794e0b215/dom/base/nsContentUtils.cpp#10111,10183
I'd rather not remove the null-checks unless NS_NewHTMLElement
aborts when it fails.

Comment 32

[Emilio Cobos Álvarez (:emilio)]

(In reply to Mats Palmgren (:mats) from comment #31)
> (In reply to Emilio Cobos Álvarez (:emilio) from comment #30)
> > CreateHTMLElement doesn't really seem fallible:
> 
> It calls NS_NewHTMLElement, which calls nsContentUtils::NewXULOrHTMLElement
> which appears to be fallible:
> https://searchfox.org/mozilla-central/rev/
> a11c128b90ea85d7bb68f00c5de52c0794e0b215/dom/base/nsContentUtils.cpp#10111,
> 10183
> I'd rather not remove the null-checks unless NS_NewHTMLElement
> aborts when it fails.

That path is only for custom elements, and a <label> can't be a custom element.

Comment 33

[Mats Palmgren (inactive)]

Oh, the first line I marked seems to be that, yes.
But not the second (return NS_ERROR_OUT_OF_MEMORY), right?

Comment 34

[Emilio Cobos Álvarez (:emilio)]

Oh, right. Though that can come from either calling NS_NewHTMLElement (which is infallible), or from calling one of the functions in https://searchfox.org/mozilla-central/rev/a11c128b90ea85d7bb68f00c5de52c0794e0b215/dom/html/nsHTMLContentSink.cpp#93, which are generated by:

  https://searchfox.org/mozilla-central/rev/a11c128b90ea85d7bb68f00c5de52c0794e0b215/dom/html/nsGenericHTMLElement.h#1244

And are also infallible.

Comment 35

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9013959 [details] [diff] [review]
part 2 - Implement cropping the filename for <input type=file>

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

Reflow / the cropping bits looks fine, just some minor concerns and questions.

::: layout/forms/nsFileControlFrame.cpp
@@ +27,5 @@
>  #include "nsContentUtils.h"
>  #include "mozilla/EventStates.h"
>  #include "nsTextNode.h"
>  
> +#ifdef ACCESSIBILITY

Unrelated, but do we support non-ACCESSIBILITY builds? Should we drop these ifdefs?

@@ +28,5 @@
>  #include "mozilla/EventStates.h"
>  #include "nsTextNode.h"
>  
> +#ifdef ACCESSIBILITY
> +#include "nsAccessibilityService.h"

Why is this needed?

@@ +148,5 @@
> +                           nsReflowStatus&    aStatus)
> +{
> +  bool done = false;
> +  // Restore the uncropped filename.
> +  HTMLInputElement::FromNode(mContent)->GetDisplayFileName(mCroppedFilename);

Why does mCroppedFilename need to be a member? Looks like it could be just a local variable on this function.

@@ +607,5 @@
> +  if (!aNotify) {
> +    if (nsIFrame* textFrame = text->GetPrimaryFrame()) {
> +      textFrame->MarkIntrinsicISizesDirty();
> +      // This is to avoid making a new Reflow request in CharacterDataChanged:
> +      textFrame->mReflowRequestedForCharDataChange = true;

This looks really hacky... Maybe we should pass an argument to CharacterDataChanged instead, or maybe create a one-off method? 

I think I still prefer this to just creating a new frame. In any case I think Jonathan or Daniel should probably stamp this part.

::: layout/forms/nsFileControlFrame.h
@@ +176,5 @@
> +
> +  /**
> +   * The rendered filename, possibly cropped to fit our inline-size.
> +   */
> +  nsAutoString mCroppedFilename;

Hmm, this grows the frame size a whole lot, do you really want to allocate it inline? Probably using nsString would be just fine, though I don't really know why it needs to be a member.

Comment 36

[Mats Palmgren (inactive)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #34)
> Oh, right. Though that can come from either calling NS_NewHTMLElement (which
> is infallible), or from calling one of the functions...

Yikes, what a mess. :)
You're right though, it seems the non-custom element path is infallible.
Hmm, I wonder if we should use NS_NewHTMLLabelElement directly instead then,
to avoid all this gobbledegook...

Comment 37

[Mats Palmgren (inactive)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #35)
> Unrelated, but do we support non-ACCESSIBILITY builds?

Yes, I use it all the time to improve build speed.

>> +#ifdef ACCESSIBILITY
>> +#include "nsAccessibilityService.h"
>
>Why is this needed?

Oops, looks like I left this in from an early version of this patch.
I'll remove it.

> This looks really hacky... Maybe we should pass an argument to
> CharacterDataChanged instead, or maybe create a one-off method? 

Sure, I can wrap this in a one-off nsTextFrame method instead.

> Hmm, this grows the frame size a whole lot, do you really want to allocate
> it inline? Probably using nsString would be just fine, though I don't really
> know why it needs to be a member.

For some reason I got the impression we need to keep the string alive
but I could be wrong.  I'll look into it...

Comment 38

[Mats Palmgren (inactive)]

Attachment: part 1 - Replace the XUL label in <input type=file> with an HTML label

(removed unnecessary OOM handling)

Comment 39

[Mats Palmgren (inactive)]

Attachment: part 2 - Implement cropping the filename for <input type=file>

As an alternative to calling CharacterDataChanged, we could just
recreate the textframe -- see the "Alternative solution" patch.
I think I slightly prefer this version though.

Comment 40

[Jonathan Kew [:jfkthame]]

(In reply to Mats Palmgren (:mats) from comment #39)
> Created attachment 9014000 [details] [diff] [review]
> part 2 - Implement cropping the filename for <input type=file>

This seems to interact weirdly with dynamic changes to max-width. I'm not sure exactly what's going on here...

Testcase:
  data:text/html,<!DOCTYPE html><input type=file style="max-width:20em">

Then open the Inspector and select the value of the max-width property, and use down-arrow to decrement it step by step until the text begins to get cropped. So far, so good. But then increment max-width again, so the cropping should go away, but it doesn't; if anything, the string continues to get shorter.


The other issue with this cropping implementation -- which was also a problem with the old XUL version -- is that it doesn't calculate correctly in the presence of contextual glyph shaping. So for example if you create a file with a name such as "‎⁨بيلبيلبيلبيلبيلبيلبيلبيلبيلبيلبيلبيلبيلبيلبيلبيلبيلبيلبيل⁩", and use the above <input type=file> element to choose it, the name will likely be cropped much shorter than would be necessary for the specified max-width. (Exact results will depend on the font being used, but I'd usually expect such a string to render much shorter as a whole than the sum of the widths of its characters measured in isolation.)

This could be fixed, or at least considerably improved, by always re-measuring the full left and right strings rather than accumulating a "total width" one character [cluster] at a time. That would be marginally more expensive, but I doubt this is really perf-critical.

Comment 41

[Jonathan Kew [:jfkthame]]

The cropping patch also breaks rather badly if the filename contains a mix of LTR and RTL characters.

Testcase: 
  data:text/html,<!DOCTYPE html><input type=file style="width:25em;border:1px solid gray;overflow:visible">

Create a file called "filenameالعربيwithالخطarabic.txt". Use the <input type=file> to select it.

Now use the Inspector to progressively reduce the width of the element, and observe what happens as the filename gets cropped.

Comment 42

[Brian Grinstead [:bgrins]]

(In reply to Jonathan Kew (:jfkthame) from comment #41)
> The cropping patch also breaks rather badly if the filename contains a mix
> of LTR and RTL characters.
> 
> Testcase: 
>   data:text/html,<!DOCTYPE html><input type=file
> style="width:25em;border:1px solid gray;overflow:visible">
> 
> Create a file called "filenameالعربيwithالخطarabic.txt". Use the <input
> type=file> to select it.
> 
> Now use the Inspector to progressively reduce the width of the element, and
> observe what happens as the filename gets cropped.

Is the behavior better or worse than the current implementation? I tried this with Nightly and that doesn't fare well:

1) cuts off part of the extension (ends in ".tx")
2) once a crop happens it flips the order of the two strings on either side of 'with'
3) once a crop happens the crop never goes away even if the input is made bigger again

Comment 8 indicates that at least (1) may be fixed.

Comment 43

[Mats Palmgren (inactive)]

(In reply to Jonathan Kew (:jfkthame) from comment #40)
> (In reply to Mats Palmgren (:mats) from comment #39)
> > Created attachment 9014000 [details] [diff] [review]
> > part 2 - Implement cropping the filename for <input type=file>
> 
> This seems to interact weirdly with dynamic changes to max-width. I'm not
> sure exactly what's going on here...

Good catch.  I think the issue might be that GetMin/PrefISize is now
reporting an intrinsic size based on the cropped value...

> The other issue with this cropping implementation -- which was also a
> problem with the old XUL version -- is that it doesn't calculate correctly
> in the presence of contextual glyph shaping.

Yeah, I pretty much just copy-pasted the old cropping code.
This could be fixed in a follow-up bug though, if we want to
remove the XUL here as a first step.  Then again, it's probably
not hard to fix so I'll look into it.

Comment 44

[Jonathan Kew [:jfkthame]]

(In reply to Brian Grinstead [:bgrins] from comment #42)
> (In reply to Jonathan Kew (:jfkthame) from comment #41)
> > The cropping patch also breaks rather badly if the filename contains a mix
> > of LTR and RTL characters.
> > 
> > Testcase: 
> >   data:text/html,<!DOCTYPE html><input type=file
> > style="width:25em;border:1px solid gray;overflow:visible">
> > 
> > Create a file called "filenameالعربيwithالخطarabic.txt". Use the <input
> > type=file> to select it.
> > 
> > Now use the Inspector to progressively reduce the width of the element, and
> > observe what happens as the filename gets cropped.
> 
> Is the behavior better or worse than the current implementation? I tried
> this with Nightly and that doesn't fare well:
> 
> 1) cuts off part of the extension (ends in ".tx")
> 2) once a crop happens it flips the order of the two strings on either side
> of 'with'
> 3) once a crop happens the crop never goes away even if the input is made
> bigger again

Yeah, the existing impl isn't great. But things get worse with the patch: once the mixed-direction text is getting cropped, I start to see garbage getting rendered (hexboxes, random Unicode chars, etc) at the end of the element.

Comment 45

[Mats Palmgren (inactive)]

Attachment: part 2 - Implement cropping the filename for <input type=file>

I fixed the nsFileControlFrame::GetMin/PrefISize so that they
now measure the uncropped filename.

I also fixed multiple issues when bidi-continuations
were involved.  They should work now.

As far as I can tell, this gives better results than our
current implementation.  Let me know if you see any
regressions.

Comment 46

[Mats Palmgren (inactive)]

Sadly, regression tests for <input type=file> are almost
non-existent.  I'm guessing it's because there's no way
to set the filename from script.  I think we need to address
this somehow, because as demonstrated here, it's very easy
to accidentally introduce regressions.

Comment 47

[Jonathan Kew [:jfkthame]]

Comment on attachment 9014516 [details] [diff] [review]
part 2 - Implement cropping the filename for <input type=file>

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

Looks much better, thanks. Now definitely an improvement on the current behavior.

(Please file a followup about further improving the substring measurement in the cropping function, as mentioned earlier; I think that'd still be worth doing but it needn't block this patch.)

::: layout/forms/nsFileControlFrame.cpp
@@ +95,5 @@
> +  using mozilla::unicode::ClusterReverseIterator;
> +  ClusterIterator leftIter(aText.Data(), aText.Length());
> +  ClusterReverseIterator rightIter(aText.Data(), aText.Length());
> +  const char16_t* dataBegin = leftIter;
> +  const char16_t* dataEnd = rightIter;

The dataBegin and dataEnd variables here look redundant; presumably we can just assign leftIter and rightIter directly to leftPos and rightPos respectively.

::: layout/forms/nsFileControlFrame.h
@@ +161,5 @@
> +   * Crop aText to fit inside aWidth using the styles of aFrame.
> +   * @return true if aText was modified
> +   */
> +  static bool CropTextToWidth(gfxContext& aRenderingContext,
> +                              nsIFrame*   aFrame,

aFrame can be a const pointer here, I believe.

(It'd be nice if aRenderingContext could be const as well, but I suspect that rabbit-hole goes too deep...)

Comment 48

[Mats Palmgren (inactive)]

Thanks, I fixed the nits as suggested and filed bug 1496998.

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6800eb348ebe
part 1 - Replace the XUL label in <input type=file> with an HTML label.  r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d95f0e0cc76
part 2 - Implement cropping the filename for <input type=file>.  r=emilio,jfkthame

Comment 50

[Narcis Beleuzu [:NarcisB]]

Backed out for causing perma failures on dynamic-max-width.html

Push link: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=pending,running,success,testfailed,busted,exception,runnable&revision=6d95f0e0cc76d43171e8c16ee04f56204da38462
Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/90853a59691e0c77014c38049bfb38b2b6ca1e16
Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=203890010&repo=mozilla-inbound&lineNumber=41144

Emilio, the fail could be caused also by https://hg.mozilla.org/integration/autoland/rev/86cc3618811a ?

Comment 51

[Emilio Cobos Álvarez (:emilio)]

Hmm, indeed. Let me land this with a tweaked fuzzy annotation for now since I'm looking at related windows widget / WR snapping issue, and the failure is this patch's fault.

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b83d37998028
part 1 - Replace the XUL label in <input type=file> with an HTML label.  r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c10bee140e0
part 2 - Implement cropping the filename for <input type=file>.  r=emilio,jfkthame

Comment 53

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/b83d37998028
https://hg.mozilla.org/mozilla-central/rev/4c10bee140e0