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

RESOLVED FIXED in Firefox 64

Status

()

P4
enhancement
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: mats, Assigned: mats)

Tracking

(Blocks: 1 bug)

Trunk
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(3 attachments, 10 obsolete attachments)

2.10 KB, patch
Details | Diff | Splinter Review
5.38 KB, patch
mats
: review+
Details | Diff | Splinter Review
16.34 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 months ago
So I hacked up a WIP for this...
(Assignee)

Comment 1

6 months ago
Assignee: nobody → mats
(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?
Flags: needinfo?(surkov.alexander)
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.
Blocks: 1446830
(Assignee)

Comment 6

6 months ago
Using a html:label instead fixed the a11y failure.
Attachment #9013055 - Attachment is obsolete: true
(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.
Flags: needinfo?(surkov.alexander)
(Assignee)

Comment 8

6 months ago
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.
(Assignee)

Comment 9

6 months ago
Attachment #9013056 - Attachment is obsolete: true
Attachment #9013146 - Attachment is obsolete: true
Attachment #9013392 - Flags: review?(emilio)
(Assignee)

Comment 10

6 months ago
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
Attachment #9013392 - Flags: review?(jfkthame)
(Assignee)

Comment 11

6 months ago
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.
Attachment #9013394 - Flags: review?(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?
Attachment #9013392 - Flags: review?(emilio) → review+
(Assignee)

Comment 13

6 months ago
> 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 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.
(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.
(Assignee)

Comment 16

6 months ago
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.
(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 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)
(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).
(Assignee)

Comment 20

6 months ago
(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...
(Assignee)

Comment 21

6 months ago
(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?
(Assignee)

Comment 22

6 months ago
Removed 'direction' and added 'unicode-bidi: plaintext'.
It places the period correctly in "No file selected." for
<input type=file dir=rtl> at least.
Attachment #9013392 - Attachment is obsolete: true
Attachment #9013392 - Flags: review?(jfkthame)
Attachment #9013418 - Flags: review?(jfkthame)
(Assignee)

Comment 23

6 months ago
Made the CropTextToWidth method static and added an aFrame param.
Attachment #9013394 - Attachment is obsolete: true
Attachment #9013394 - Flags: review?(emilio)
Attachment #9013420 - Flags: review?(emilio)
(Assignee)

Comment 25

6 months ago
BTW, the comment in forms.css comes from bug 762270.
(Assignee)

Comment 26

6 months ago
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...
Attachment #9013418 - Flags: review?(jfkthame)
(Assignee)

Updated

6 months ago
Attachment #9013420 - Flags: review?(emilio)
(Assignee)

Comment 27

6 months ago
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.
Attachment #9013418 - Attachment is obsolete: true
Attachment #9013420 - Attachment is obsolete: true
Attachment #9013955 - Flags: review?(emilio)
(Assignee)

Updated

6 months ago
Attachment #9013955 - Attachment description: Replace the XUL label in <input type=file> with an HTML label → part 1 - Replace the XUL label in <input type=file> with an HTML label
(Assignee)

Comment 28

6 months ago
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.)
Attachment #9013959 - Flags: review?(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.
Attachment #9013955 - Flags: review?(emilio) → review+
(Assignee)

Comment 31

6 months ago
(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.
(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.
(Assignee)

Comment 33

6 months ago
Oh, the first line I marked seems to be that, yes.
But not the second (return NS_ERROR_OUT_OF_MEMORY), right?
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 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.
Attachment #9013959 - Flags: review?(emilio)
(Assignee)

Comment 36

6 months ago
(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...
(Assignee)

Comment 37

6 months ago
(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...
(Assignee)

Comment 38

6 months ago
(removed unnecessary OOM handling)
Attachment #9013955 - Attachment is obsolete: true
Attachment #9013998 - Flags: review+
(Assignee)

Comment 39

6 months ago
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.
Attachment #9013959 - Attachment is obsolete: true
Attachment #9014000 - Flags: review?(jfkthame)
(Assignee)

Updated

6 months ago
Summary: Replace the XUL label in <input type=file> with an anonymous <span> → Replace the XUL label in <input type=file> with an HTML label
(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.
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.
(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.
Flags: needinfo?(jfkthame)
(Assignee)

Comment 43

6 months ago
(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.
(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.
Flags: needinfo?(jfkthame)
(Assignee)

Comment 45

6 months ago
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.
Attachment #9014000 - Attachment is obsolete: true
Attachment #9014000 - Flags: review?(jfkthame)
Attachment #9014516 - Flags: review?(jfkthame)
(Assignee)

Comment 46

6 months ago
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 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...)
Attachment #9014516 - Flags: review?(jfkthame) → review+
(Assignee)

Comment 48

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

Comment 49

6 months ago
Pushed by mpalmgren@mozilla.com:
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
(Assignee)

Updated

6 months ago
Blocks: 1365434
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.
Flags: needinfo?(emilio)

Comment 52

6 months ago
Pushed by emilio@crisal.io:
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

Updated

6 months ago
Blocks: 1161482

Comment 53

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b83d37998028
https://hg.mozilla.org/mozilla-central/rev/4c10bee140e0
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox64: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64

Updated

5 months ago
Depends on: 1500530
(Assignee)

Updated

5 months ago
Blocks: 1502493
You need to log in before you can comment on or make changes to this bug.