Closed Bug 1329907 Opened 3 years ago Closed 3 years ago

regression: setCellText is now called with empty value after startEditing

Categories

(Core :: XUL, defect, major)

defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox50 --- unaffected
firefox51 + wontfix
firefox52 --- fixed
firefox53 --- verified

People

(Reporter: mimecuvalo, Assigned: enndeakin)

References

Details

Attachments

(2 files)

Attached file test.xul
Hi, I'm the developer of the FireFTP extension and my users have been complaining that they can't rename files/create files anymore in the latest dev versions of Firefox.

What happens:
- call made to startEditing in the tree
- setCellText immediately called with value == ''
- confuses FireFTP since it thinks editing is done

Expected behavior:
- call made to startEditing
- setCellText called *only* after hitting enter/escape (regular triggers for finishing editing a cell)

Attached is a test tree (example pulled from MDN docs).  You can see that when you start editing the console shows empty values when starting editing.
It's not broken in Firefox 50 afaict but the dev version (52) exhibits the behavior.
This is a regression from 1113581. The attached patch is a better fix that doesn't change the actual cell contents. Instead, it just sets the opacity of the cell being edited to 0 and clears the background and border.

The remainder of the patch fixes the setting of the current column when you double-click.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Attachment #8825436 - Flags: review?(jaws)
[Tracking Requested - why for this release]: functional regression introduced in 51
https://hg.mozilla.org/integration/mozilla-inbound/rev/cafadfb28b156db4602ae3e08139eaf1c8e3c435
Bug 1329907, use opacity to clear the treecell text while editing instead of changing the value, r=jaws
https://hg.mozilla.org/mozilla-central/rev/cafadfb28b15
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Track 51+ as regression.
Hi Florin,
Can you help find someone to verify if this is fixed as expected?
Flags: qe-verify+
Flags: needinfo?(florin.mezei)
(In reply to Gerry Chang [:gchang] from comment #6)
> Track 51+ as regression.
> Hi Florin,
> Can you help find someone to verify if this is fixed as expected?

Added to our todo list, leaving the ni? in place as a reminder.
Flags: needinfo?(florin.mezei) → needinfo?(andrei.vaida)
Too late for 51. Mark 51 won't fix.
Hi , fix new FireFTP error please all complaining about that :

[code]
"Error message=TypeError: event.dataTransfer.types.includes is not a function
URL=chrome://fireftp/content/js/etc/dragDrop.js
Line Number=29"
[/code]

Lot of reviews complaining about this , need support to fix this , we cant drag and drop files !!!!

https://addons.mozilla.org/en-US/firefox/addon/fireftp/reviews/
Hi Liz, Gerry, fyi this was tracked for 51. I haven't weighed in on the risk involved in taking this system but this might be something we could consider taking in case we spin another RC. Will let you guys decide. Thanks!
Flags: needinfo?(lhenry)
Flags: needinfo?(gchang)
Fwiw, for other developers that will be hitting this problem in their extensions, here is my workaround:

setCellText : function(row, col, val) {
    // XXX Firefox 51 has a regression that calls setCellText immediately
    // upon calling startEditing. Hacks below to see if startEditing is in the
    // call stack.
    if (col && !val) {
      try {
        throw Error('blah');
      } catch(ex) {
        if (ex.stack && ex.stack.indexOf('startEditing') != -1) {
          return;
        }
      }
    }

    <rest of original code>
Reproduced the bug with an affected build using FireFTP 2.0.28 -- creating or renaming file actions confirmed as broken.

This is verified fixed on 53.0a1 (2017-01-19), creating and renaming files now works as expected.

Tested this on Windows 10 x64, Ubuntu 14.04 x64 and Mac OS X 10.11.6, with both FireFTP 2.0.28 and 2.0.31.

I'll keep a close eye on this, we'll do our best to verify it on the other channels as well, once landed.
Status: RESOLVED → VERIFIED
Flags: needinfo?(andrei.vaida)
Too late to get this into a 51 RC build unless we delayed the release. We could take the fix for 52, though. I hope the workaround is sufficient for 51, and I'll keep the bug tracked for now in case the problem turns out to be more widespread.
Flags: needinfo?(lhenry)
Neil can you request uplift to aurora? Thanks.
Flags: needinfo?(enndeakin)
Comment on attachment 8825436 [details] [diff] [review]
Clear appearance of cells while editing

Approval Request Comment
[Feature/Bug causing the regression]: 1113581
[User impact if declined]: compatibility issue with FireFTP, possibly other extensions
[Is this code covered by automated tests?]: no, but manually tested
[Has the fix been verified in Nightly?]: to be done
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no, it is an improved fix 
[Why is the change risky/not risky?]: it partially reverts the regressing bug and fixes it with a css-style only change
[String changes made/needed]: no
Flags: needinfo?(enndeakin)
Attachment #8825436 - Flags: approval-mozilla-aurora?
Flags: needinfo?(gchang)
Comment on attachment 8825436 [details] [diff] [review]
Clear appearance of cells while editing

fix recent regression, beta52+, should be in b2
Attachment #8825436 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.