Closed Bug 1328500 Opened 5 years ago Closed 5 years ago

Support input field when user clicks on cell for TreeView

Categories

(DevTools :: Shared Components, defect, P1)

defect

Tracking

(firefox53 fixed)

RESOLVED FIXED
Firefox 53
Iteration:
53.4 - Jan 9
Tracking Status
firefox53 --- fixed

People

(Reporter: rickychien, Assigned: rickychien)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [netmonitor])

Attachments

(4 files)

VariablesView.jsm has a ability to enter an input mode and make it possible to user to select a long string easily.

This is an essential feature in ParamsPanel, CookiesPanel, HeadersPanel and ResponsePanel. So, we should support this in TreeView as well.
Honza,

This patch is hard to test without params panel. However, I extract this patch from the diff in https://reviewboard.mozilla.org/r/99644/diff/31/, and you can apply this link and check visual / UX effect in params panel.
Iteration: --- → 53.4 - Jan 9
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [netmonitor][triage] → [netmonitor]
Comment on attachment 8823540 [details]
Bug 1328500 - Support input field in TreeView

https://reviewboard.mozilla.org/r/102066/#review102510

Looks good overall, just couple of comments.
Honza

::: devtools/client/shared/components/tree/tree-header.js:85
(Diff revision 1)
>              className: classNames.join(" "),
>              style: cellStyle,
>              key: col.id},
> -            div({ className: visible ? "treeHeaderCellBox" : "" },
> -              visible ? col.title : ""
> -            )
> +            visible ? div({ className: "treeHeaderCellBox"},
> +              col.title
> +            ) : "",

Use null instead of an empty string.

::: devtools/client/shared/components/tree/tree-view.css:21
(Diff revision 1)
>  
>  /******************************************************************************/
>  /* TreeView Table*/
>  
> +.treeTable .treeRow {
> +  height: 21.5px;

Why the height is hardcoded here? We should always try to avoid this.
Attachment #8823540 - Flags: review?(odvarko)
Comment on attachment 8823540 [details]
Bug 1328500 - Support input field in TreeView

https://reviewboard.mozilla.org/r/102066/#review102510

> Why the height is hardcoded here? We should always try to avoid this.

Hardcode is because after input appears instead of span, you can see tree row will become higher than before. (height will grow from 19px to 19.5px after input appears)
Honza, if you don't know what I mean, you can apply my patch in comment 2 and remove the heigh: 21.5px to see the actual effect.
(In reply to Ricky Chien [:rickychien] from comment #4)
> Comment on attachment 8823540 [details]
> Bug 1328500 - Support input field in TreeView
> 
> https://reviewboard.mozilla.org/r/102066/#review102510
> 
> > Why the height is hardcoded here? We should always try to avoid this.
> 
> Hardcode is because after input appears instead of span, you can see tree
> row will become higher than before. (height will grow from 19px to 19.5px
> after input appears)

I am experiencing the same problem on Win 10 even if the height is still hardcoded.
(I forgot to mention that before)

Is it because of the input's outline/border?

Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #7)
> I am experiencing the same problem on Win 10 even if the height is still
> hardcoded.
> (I forgot to mention that before)
> 
> Is it because of the input's outline/border?

No, input field is taller than span element, so the tree row will be expanded to fit input height. My current solution is to set a fixed input height 15px, so tree row will not be affected unexpectedly.

Does it work in your Win 10?
Flags: needinfo?(odvarko)
Attached image params-panel.png
(In reply to Ricky Chien [:rickychien] from comment #9)
> Does it work in your Win 10?

No, the input is taller than the span and so, clicking on a value makes it bigger (since the input field appears). I tried to set height of the input (using the '.treeTable .treeValueCell input' selector) to 13px, which makes the input height the same as the span. But, there are some padding/margin that prevents the input field from taking the entire available area within the row. See the attached screenshot.

Honza
Flags: needinfo?(odvarko)
I think the easiest way is to set a taller heigh for tree row, so make it possible to render properly. Set 21.5px is enough in macOS but it still break on Win. 

Honza, could you give me an appropriate height for your Win 10?
Flags: needinfo?(odvarko)
Attached patch bug1317650.patchSplinter Review
Attaching a patch that solves the problem on Win10

It's removing top & bottom padding for td if the input is enabled.

Does it work on you machine?

Honza
Flags: needinfo?(odvarko)
Attached image source-editor.png
One more thing. Could the source editor always take the rest of the (vertical) available space? See the attached screenshot.

Honza
Great! This patch works on macOS too. I've updated my patch.

P.S. I separated Properties View from Params panel to wrap search filter + tree view + source editor + rep together. We can discuss the issue on bug 1328828.
(In reply to Jan Honza Odvarko [:Honza] from comment #13)
> Created attachment 8824095 [details]
> source-editor.png
> 
> One more thing. Could the source editor always take the rest of the
> (vertical) available space? See the attached screenshot.
> 
> Honza

I'm going to move this issue and attachment to bug 1328828.
Blocks: 1328828
Comment on attachment 8823540 [details]
Bug 1328500 - Support input field in TreeView

https://reviewboard.mozilla.org/r/102066/#review103370

Removing myself from the review till bug 1328828 is solved.

Honza
Attachment #8823540 - Flags: review?(odvarko)
Comment on attachment 8823540 [details]
Bug 1328500 - Support input field in TreeView

https://reviewboard.mozilla.org/r/102066/#review103376

Yep, looks good now.

R+

Thanks,
Honza
Attachment #8823540 - Flags: review+
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fd4b40c65e7e
Support input field in TreeView r=Honza
https://hg.mozilla.org/mozilla-central/rev/fd4b40c65e7e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Blocks: 1329575
Flags: qe-verify? → qe-verify-
Depends on: 1352777
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.