Closed Bug 1039482 Opened 10 years ago Closed 9 years ago

New file text field is not properly aligned

Categories

(DevTools Graveyard :: WebIDE, defect)

x86_64
All
defect
Not set
normal

Tracking

(firefox44 fixed)

VERIFIED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: paul, Assigned: arjun.jain, Mentored)

References

Details

(Whiteboard: [good first bug][lang=css][bugday-20151028])

Attachments

(3 files, 7 obsolete files)

See screenshot.
Here (on Mac at least) it additionally uses a serif font instead of sans like the rest of the list as well.
Mentor: jryans
Whiteboard: [good first bug][lang=css]
Would be happy to work on it as my first contribution, please give me more info on this. Thank you.
Sorry for the delay in replying!  I would suggest using "needinfo" below and setting to "mentor", to ensure your question is not missed.

To get started, check out our hacking page[1].  

The code that handles the new command[2] call |tree.promptNew|[3], which creates an |InplaceEditor|[4] as the input.  |InplaceEditor| will copy text styles[5] from the element passed as |element| in it's options.

So, we'll want to alter the styling of the input this editor creates.

I'll be out next week, so you can also ask :bgrins on IRC or needinfo? while I am away.

[1]: https://wiki.mozilla.org/DevTools/Hacking
[2]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/projecteditor/lib/plugins/new/new.js
[3]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/projecteditor/lib/tree.js#243
[4]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/inplace-editor.js
[5]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/inplace-editor.js#263
I'm trying to understand what the DOM looks like in the tree element. Is there a good way to use the FF devtools against the WebIDE itself for debugging purposes?
Flags: needinfo?(jryans)
(In reply to Gregory Gross from comment #4)
> I'm trying to understand what the DOM looks like in the tree element. Is
> there a good way to use the FF devtools against the WebIDE itself for
> debugging purposes?

Yes, this now possible with a recently added feature!

1. Open WebIDE to some project
2. Switch back to a Firefox window (you need it to access the Tools menu)
3. Tools -> Web Developer -> Browser Toolbox
  * See the MDN page[1] if you don't see this menu option
4. This starts toolbox in a remote process aimed at the entire browser
5. Click the window / frame selection icon[2]
6. Choose "chrome://webide/content/webide.xul" to aim the toolbox at all of WebIDE
7. The inspector now points at the DOM for WebIDE

Also, you may wish to choose a different frame, like "chrome://browser/content/devtools/projecteditor.xul", which is more focused on the file tree and editor area.

To inspect a particular element, you click the top-left "pick" icon in the browser toolbox, and then click an element in WebIDE to focus it in the inspector.

[1]: https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox#Enabling_the_Browser_Toolbox
[2]: https://github.com/mozilla/gecko-dev/blob/master/browser/themes/shared/devtools/images/command-frames.png
Flags: needinfo?(jryans)
I've posted a patch to reviewboard:

https://reviewboard.mozilla.org/r/5581/

Can you review it Ryan? If not, how do I find someone who can?
Flags: needinfo?(jryans)
Attached image indent3-1039482.png (obsolete) —
I've attached a screenshot of what it looks like with my patch.
(In reply to Gregory Gross from comment #6)
> I've posted a patch to reviewboard:
> 
> https://reviewboard.mozilla.org/r/5581/
> 
> Can you review it Ryan? If not, how do I find someone who can?

Wow, I am sorry for the delay here!  This seems to have slipped through my usual workflow for tracking requests.  I'll take a look now!
Flags: needinfo?(jryans)
For future uses of MozReview, you can add a reviewer's name with "pencil" icon.  This should attach the review to the bug and request the review.

I'll try to attach it to this bug now.
Hmm, the review request seems a little confused.  There should have been a "parent" one to publish, which is the last URL printed out when using the mozreview Mercurial extension.
I guess my MozReview comment did not get posted here, since the bug link seems not quite right...

For me, it seems slightly more aligned than before your patch, but it's still not quite right.  Also, I noticed that other operations, like rename, that show the input are also not aligned correctly.  Can you check rename as well on your system?

I'll attach my screenshots to the bug.  It could be that this is OS dependent, since you are on Linux, and I am on a Mac.  If you don't have access to a Mac, I can try to continue your work for Mac.
Attached image 1039482-without-patch.png (obsolete) —
Before the MozReview patch
Attached image 1039482-with-patch.png (obsolete) —
After the MozReview patch
Attached patch 0272-patch-for-bug-1039482.patch (obsolete) — Splinter Review
Hi! We noticed this bug hadn't had any attention in a while so we gave it a go. In addition to fixing alignment, we also fixed the font in the edit-box which is the change in inplace-editor.js and now the "rename" file edit-box is visually consistent with the "new" file edit-box. Let us know what you think.
Comment on attachment 8661036 [details] [diff] [review]
0272-patch-for-bug-1039482.patch

Thanks for the patch, I'll take a look soon!  Be sure to set review or feedback flags to my name on future patches to make sure they aren't missed.  If you have questions, you can set the needinfo flag to me at the bottom of the bug.
Attachment #8661036 - Flags: review?(jryans)
Comment on attachment 8661036 [details] [diff] [review]
0272-patch-for-bug-1039482.patch

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

Well, it appears to function nicely, so that's great!

Please update your patch commit message, as in the example from this guide[1].

After looking at the patch, I feel like I don't know this area as well as I should.  Also, I am not sure what impact the inplace-editor change may have, since it's used elsewhere.

Brian, could you review this?  I think you are more familiar with these parts.

[1]: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions
Attachment #8661036 - Flags: review?(jryans) → review?(bgrinstead)
Just a heads up - I've seen the request but have been traveling and haven't been able to review it.  I'll look at it early next week.
Comment on attachment 8661036 [details] [diff] [review]
0272-patch-for-bug-1039482.patch

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

Hi, and thanks for the patch.  The test browser_projecteditor_rename_file.js is going to need to be updated to pass with the changes applied, you can run it lo`cally via `./mach mochitest devtools/client/projecteditor/test/browser_projecteditor_rename_file.js`.  And the whole projecteditor test folder can be run with ./mach mochitest devtools/client/projecteditor`.

Also, you've coincided with some folder structure changes in the project so you'll need to pull down the latest code and apply a rebased version of the patch (which I will attach after this).  Also, a tip when working on the project editor that I'm not sure if you are aware of - you can test changes locally by loading chrome://devtools/content/projecteditor/chrome/content/projecteditor-loader.xul in a tab (which creates a temp folder and lets you use the devtools toolbox when working with it).

::: browser/devtools/projecteditor/lib/tree.js
@@ +324,4 @@
>          editor.input.select();
>        },
>        done: function(val, commit) {
> +        if(val==initial){

Nit: `if (val === initial) {`

::: browser/devtools/shared/inplace-editor.js
@@ +280,4 @@
>    },
>  
>    _createInput: function() {
> +

Nit: can you revert the whitespace changes in this file

@@ +1220,4 @@
>  function copyTextStyles(from, to) {
>    let win = from.ownerDocument.defaultView;
>    let style = win.getComputedStyle(from);
> +  to.style.font = style.font;

I'm curious why this was changed, can you elaborate?
Attachment #8661036 - Flags: review?(bgrinstead) → feedback+
Rebased the patch for you so you can apply this one if you'd like
Attachment #8579271 - Attachment is obsolete: true
Attachment #8582053 - Attachment is obsolete: true
Attachment #8582057 - Attachment is obsolete: true
Attachment #8582058 - Attachment is obsolete: true
Assignee: nobody → arjun.jain
Status: NEW → ASSIGNED
Here is a new patch with the stylistic fixes and working tests. To answer your question about our change in "inplace-editor.js" we noticed as per an earlier comment that the font in the editor box was not consistent with the filename font. We tracked this issue down to the function "copyTextStyles" where not all of the needed attributes were being copied so the font was not properly changed. This solution gave us the desired font style.
Attachment #8661036 - Attachment is obsolete: true
Attachment #8666575 - Flags: review?(bgrinstead)
(In reply to Arjun Jain, Chris Bernt, Wes Valentine from comment #21)
> Created attachment 8666575 [details] [diff] [review]
> 0005-Bug-1039482-Changed-font-copying-DOM-structure-chang.patch
> 
> Here is a new patch with the stylistic fixes and working tests. To answer
> your question about our change in "inplace-editor.js" we noticed as per an
> earlier comment that the font in the editor box was not consistent with the
> filename font. We tracked this issue down to the function "copyTextStyles"
> where not all of the needed attributes were being copied so the font was not
> properly changed. This solution gave us the desired font style.

Hm, so it looks like the fix is because of the removal of the font-family copying.  For some reason `styles.fontFamily` on OSX comes through as '\.Helvetica Neue DeskInterface', which doesn't seem to apply properly - probably due to the inherited 'font: message-box' rule on the container.  I can't figure out exactly what's going on there.  I saw Bug 931040 which seems sort of related, but I haven't been able to track this issue down.  I made a simple test case here: http://jsfiddle.net/3rq1om3j/1/, but I think that's getting beyond the scope of this bug.

We can't remove the functionality as the patch currently does, because that causes the rule-view editors to not match the rule text (you can see that in the "Rules" panel in the Inspector tab).

The computed 'font' value on the container seems to always be "" so I don't think that line is doing anything (as I've checked in the console on chrome://devtools/content/projecteditor/chrome/content/projecteditor-loader.xul).  Can you confirm that not calling copyTextStyles at all gives the desired style?

My suggestion is we add a new option to the inplace editor, maybe called preserveTextStyles.  If that's true, then don't call copyTextStyles at all.
Comment on attachment 8666575 [details] [diff] [review]
0005-Bug-1039482-Changed-font-copying-DOM-structure-chang.patch

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

See Comment 22 also

::: devtools/client/projecteditor/lib/tree.js
@@ +246,5 @@
>      let parentContainer = this._containers.get(parent);
>      let item = this.doc.createElement("li");
>      item.className = "child";
> +
> +    let inputholder = this.doc.createElementNS(HTML_NS, "div");

Can you please pull this stuff out into a function like so:

createInputContainer: function() {
  let inputholder = this.doc.createElementNS(HTML_NS, "div");
  inputholder.className = "child entry";

  let expander = this.doc.createElementNS(HTML_NS, "span");
  expander.className = "arrow expander";
  expander.setAttribute("invisible", "");
  inputholder.appendChild(expander);

  let placeholder = this.doc.createElementNS(HTML_NS, "div");
  placeholder.className = "child";
  inputholder.appendChild(placeholder);

  return {inputholder, placeholder};
},

Then it could be called as: 

    let {inputholder,placeholder} = this.createInputContainer();
    item.appendChild(inputholder);

and, later:

    let {inputholder,placeholder} = this.createInputContainer();
    item.insertBefore(inputholder, originalText);
    item.removeChild(originalText);
Attachment #8666575 - Flags: review?(bgrinstead)
Here are the changes as per your suggestions. We didn't change the copyTextStyles call in the _autosize function on line 351, because it seemed to work fine correctly without it. Let us know what you think.
Attachment #8666575 - Attachment is obsolete: true
Attachment #8670633 - Flags: review?(bgrinstead)
Comment on attachment 8670633 [details] [diff] [review]
0005-Bug-1039482-Changed-font-copying-DOM-structure-chang.patch

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

This looks good to me, thanks!  Can you please make the minor update noted below and then also update the commit message to say what the patch is doing specifically?  In this case, something like "Bug 1039482 - Properly position and style the file name edit field in the projecteditor;r=bgrins" should do the trick.  You can reupload the patch with an r+ flag after that

::: devtools/client/shared/inplace-editor.js
@@ +94,4 @@
>   *      defaults to false
>   *    {Boolean} trimOutput: Should the returned string be trimmed?
>   *      defaults to true
> + *    {Boolean} preserveTextStyles: Calls copyTextStyles when false, does not when true.

Suggested comment:

 *    {Boolean} preserveTextStyles: If true, do not copy text-related styles
 *              from `element` to the new input.
 *      defaults to false
Attachment #8670633 - Flags: review?(bgrinstead) → review+
Attachment #8670633 - Attachment is obsolete: true
Attachment #8671994 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/48730a306b25
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
¡Hola Paul!

I've verified this is now properly aligned on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0 ID:20151027030239 CSet: 9a8f2342fb3116d23989087e026448d38a3768c5

I've noticed that right clicking on the blank area underneath the file list does not give a context menu with "New...". Is this a bug or a feature?

¡Gracias!
Alex
Status: RESOLVED → VERIFIED
Flags: needinfo?(paul)
Whiteboard: [good first bug][lang=css] → [good first bug][lang=css][bugday-20151028]
(In reply to alex_mayorga from comment #30)
> I've noticed that right clicking on the blank area underneath the file list
> does not give a context menu with "New...". Is this a bug or a feature?

Seems work filing a new bug for this.  Thanks!
Flags: needinfo?(paul)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #31)
> Seems work filing a new bug for this.  Thanks!

Seems *worth*...
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: