Closed Bug 1241059 Opened 8 years ago Closed 5 years ago

Inspector doesn't scroll to the element if I click closing tag of an already selected element

Categories

(DevTools :: Inspector, defect, P2)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(firefox46 affected, firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox46 --- affected
firefox53 --- fixed

People

(Reporter: arni2033, Assigned: msfr-develop, Mentored)

References

(
URL
)

Details

(Keywords: good-first-bug)

Attachments

(1 file, 4 obsolete files)

Bug1241059_prt1.patch
1.78 KB, patch
gl
: review+
Details | Diff | Splinter Review 
>>>   My Info:   Win7_64, Nightly 46, 32bit, ID 20160118030338
STR_1:
1. Open the following "data:" url or click the URL in the form above
>   data:text/html,<body><style>div::before,div::after{content:""}</style><script>var str=""; for(i=0;i<99;i++)document.write('<div><\/div>')</script>
2. Open Devtools -> Inspector, click "<body>" to select that element
3. Scroll markup to the bottom, click "</body>"


STR_2   (the same as STR_1, but more common):
1. Open the same "data:" url or click the URL in the form above
>   data:text/html,<body><style>div::before,div::after{content:""}</style><script>var str=""; for(i=0;i<99;i++)document.write('<div><\/div>')</script>
2. Open Devtools -> Inspector, Click dropmarker near "<body>" to collapse child nodes list
   Alt+Click the dropmarker near "<body>" to expand all descendant nodes
3. Scroll markup to the bottom. Make sure that the first visible in markup string is not "<div>".
   Otherwise resize devtools toolbox by 2 lines and scroll markup to the bottom again.
4. Click on the first (from top) string "</div>"  [<div> element is selected and scrolled into view]
5. Scroll markup to the bottom. Click on the first (from the top) string "</div>"


AR: Nothing happens after the last step (STR_1 - Step 3, STR_2 - Step 5)
ER: The clicked element should be scrolled into view (in markup)

Note: I reported bug 1197267 and a few similar bugs about "scroll into view on click" mechanism
      and I'm not sure in which order they should be resolved.
Triaging (filter on CLIMBING SHOES).
Priority: -- → P2
Whiteboard: [btpp-fix-later]
So, to be clear, the expected result is that clicking on the closing </body> tag (in STR_1) should scroll back up to the top so that the opening <body> tag is visible, right?

It's strange because it does work this way for me the first time I try, so I can do it once, and then if I try again after scrolling down again, it doesn't work anymore.

For sure we should be consistent and either always scroll back to the corresponding opening tag, or never scroll, but not have this half broken state where it sometimes scrolls only.

I'm not entirely convinced that we should scroll in fact. Is scrolling back up a useful feature here, or could it be more of an annoyance if you, say, click by accident on a closing tag but you really didn't meant to loose your current scrolling position.

Let's ask Gabriel and Julian for their point of views.

In any case, whatever the decision taken, this should be a relatively easy bug to fix if anyone wants to try it (make sure you go over our contribution guide first though: https://developer.mozilla.org/en-US/docs/Tools/Contributing ).
Flags: needinfo?(jdescottes)
Flags: needinfo?(gl)
Keywords: good-first-bug
(In reply to Patrick Brosset <:pbro> from comment #2)
> could it be more of an annoyance if you, say, click by accident on a closing tag but you really
> didn't meant to loose your current scrolling position.
Actually, yes. But there're a few more bugs here. I think it doesn't make sense to file any of them
separately yet, because it's unclear in which way this bug will be fixed. And because STR is the same

1) So far there's the only way to scroll element into view if I only see its closing tag:
   to click on the closing tag, then (if necessary) click on that element in breadcrumbs.
   If breadcrumbs will be removed, then there will be no reliable way to scroll element into view

2) It's not possible to say if element is selected if I only see its closing tag.
   Recently implemented 1px line to the left from selected element (in markup) is not visible if
   closing tag is in the first visible line in markup. Preferably, closing tag should be selected
  [OR]  1px line should continue to the closing tag to indicate that the element is already selected

3) In STR_2 after Step 5 closing tag </div> doesn't look "hovered" when I move mouse over it.
(In reply to arni2033 from comment #3)
> (In reply to Patrick Brosset <:pbro> from comment #2)
> > could it be more of an annoyance if you, say, click by accident on a closing tag but you really
> > didn't meant to loose your current scrolling position.
> Actually, yes. But there're a few more bugs here. I think it doesn't make
> sense to file any of them
> separately yet, because it's unclear in which way this bug will be fixed.
> And because STR is the same
> 
> 1) So far there's the only way to scroll element into view if I only see its
> closing tag:
>    to click on the closing tag, then (if necessary) click on that element in
> breadcrumbs.
>    If breadcrumbs will be removed, then there will be no reliable way to
> scroll element into view
You're right, we need a way to scroll the selected element into view. Let's assume we leave the breadcrumbs where it is now (we have no plans to get rid of it right now), that means we could remove the scrollIntoView when the closing tag is clicked. I'd still like to have other people's opinions on this, but I prefer this option.
> 2) It's not possible to say if element is selected if I only see its closing
> tag.
>    Recently implemented 1px line to the left from selected element (in
> markup) is not visible if
>    closing tag is in the first visible line in markup. Preferably, closing
> tag should be selected
>   [OR]  1px line should continue to the closing tag to indicate that the
> element is already selected
I agree that the line should extend all the way to the bottom of the closing tag.
> 3) In STR_2 after Step 5 closing tag </div> doesn't look "hovered" when I
> move mouse over it.
Right, that's another problem that needs to be fixed here.
1) I agree with some annoyance brought up in pbro's comment 2 that clicking on the closing tag and having it scrolled into view could be a possible annoyance if it was an accidental click. I think the closing tag should be selectable, and to scroll into view click on the breadcrumbs. Assuming we don't make the closing tag selectable, we should fix the scroll into view consistency problems.

2) Yes I agree the line should extend all the way to the bottom of the closing tag.
Flags: needinfo?(gl)
See Also: → 1197267
Summary: Inspector doesn't scroll to the element if I click an already selected element → Inspector doesn't scroll to the element if I click closing tag of an already selected element
Clearing NI? for Julian since Gabriel already answered.
Flags: needinfo?(jdescottes)
Let's get this moving.
There are 2 things to fix here:

1) when clicking on a closing tag, this should not scroll back up to the corresponding opening tag. Instead, it should mark the closing tag as selected

2) the vertical line should extend all the way down to the closing tag
http://searchfox.org/mozilla-central/source/devtools/client/themes/markup.css#142

3) when 1) is fixed, I believe this would go away.
Mentor: gl
I have fixed prt. 2 of this bug, but i'm unsure where to go from here. The Mozilla doc (How to submit a patch) says one should verify with the module-owner that the proposed change is acceptable. So, do you need some documentation about my changes before creating and submitting the patch?

I hope that it is ok to ask such questions directly inside the bug.

Thank you and have a nice day,
Marco
Flags: needinfo?(gl)
(In reply to msfr-develop from comment #8)
> I have fixed prt. 2 of this bug, but i'm unsure where to go from here. The
> Mozilla doc (How to submit a patch) says one should verify with the
> module-owner that the proposed change is acceptable. So, do you need some
> documentation about my changes before creating and submitting the patch?
> 
> I hope that it is ok to ask such questions directly inside the bug.
> 
> Thank you and have a nice day,
> Marco

Hey Marco,

You can attach the patch file and set the reviewer to myself. I would expect a commit message that looks like:
Bug 123456 - Change this thing to work better by doing something. r=reviewers
That would give me a simple doc about what was done in the changes. You can also further add a bugzilla comment after attaching the patch explaining what was done to help simplify the review process.

Let me know if you need help generating a patch and attaching it on irc.

Thanks,
Gabriel
Flags: needinfo?(gl)
Attached patch Bug1241059_prt2.patch (obsolete) — — Splinter Review 
Hi Gabriel, if using the bzexport extension the patch creation always failed, so i choosed the 'not so nice way' and used 'hg export'. I hope that this method works also for you. 

Best whishes,
Marco
Flags: needinfo?(gl)
Attachment #8807949 - Flags: review+
Attachment #8807949 - Flags: feedback+
Comment on attachment 8807949 [details] [diff] [review]
Bug1241059_prt2.patch

Just setting the necessary flags for a review - you usually set r? with the reviewer's :name. 

Just looking quickly at the patch - the format of the patch looks fine. The commit message could be refined to be shorter. We usually want a short commit message for the first line for the commit logs and then a longer commit message in the subsequent lines if you needed.
Flags: needinfo?(gl)
Attachment #8807949 - Flags: review?(gl)
Attachment #8807949 - Flags: review+
Attachment #8807949 - Flags: feedback+
Attached patch Bug1241059_prt2.patch (obsolete) — — Splinter Review 
Amended the commit message to the suggested format.
Attachment #8807969 - Flags: review?(gl)
Attachment #8807969 - Flags: feedback+
Comment on attachment 8807949 [details] [diff] [review]
Bug1241059_prt2.patch

You can usually select the previous patch and mark it as obsolete when you upload a new patch.
Attachment #8807949 - Attachment is obsolete: true
Attachment #8807949 - Flags: review?(gl)
Comment on attachment 8807969 [details] [diff] [review]
Bug1241059_prt2.patch

Hey Marco,

As the patch submitter, you don't usually need to set more than one of review or feedback flag. Feedback is usually use if you are looking for feedback on a work in progress patch. Review is when you have a patch that you believe is ready to land and needs a review from the module owner. You will want to stick to '?' when requesting a review or feedback or someone, and the reviewer or feedback individual will usually set a '+' if the code reviewed is ready to ship or '-' if it isn't. After a patch review has been '+', any subsequent modifications to address reviewer feedback can carry forward the '+' and does not need another review from the reviewer. Just wanted to make sure you know how to make the most use of bugzilla for getting patches feedback or reviewed.

For more info, https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed
Attachment #8807969 - Flags: feedback+
Comment on attachment 8807969 [details] [diff] [review]
Bug1241059_prt2.patch

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

I know we said this was part 2 of the problem to be fixed in the bug, but as a patch that is landable first, we should rename this to Part 1. In your commit message, you should state:
Bug 1241059 - Part 1: Extend the markup outline to the closing tag r=gl

I think this should be sufficient for the commit message.

::: devtools/client/themes/markup.css
@@ +139,5 @@
>    padding-left: 2px;
>  }
>  
> +.tag-line[selected] + .children,
> +.tag-line[selected] ~ .tag-line.flash-out {

We don't need the flash-out selector in this case. .tag-line[selected] already provides enough specificity for the tag-line that we care about. flash-out is actually only responsible for the animation. I also noticed that the flash-out classes isn't actually properly removed.

@@ +145,5 @@
>    background-repeat: no-repeat;
> +  background-size: 1.5px 100%;
> +}
> +
> +.tag-line[selected] + .children {

So, it turns out we really want to keep background-position and border-left in .tag-line[selected] + .children, .tag-line[selected ~ .tag-line. The ideal solution actually needed to add .tag-line[selected] ~ .tag-line to .tag-line[selected] + .children in line 142.

However, we do need to modify the background-position for .tag-line[selected] + .children and .tag-line[selected] ~ .tag-line to apply background-position-x: -6px for both. We want to keep the original style which adds a 2px padding at the top and bottom (not actually done using padding but using background-position-y of 2px and shortening the background-size.

However, there is still a problem when we apply a background style to tag-line because there is a flash-out class that applies a transition to the background style. I don't believe we need a flash-out for the closing tag in this case. So, the next thing we need to do is remove this class for tag-line closing tags.

@@ +151,5 @@
>    border-left: 6px solid transparent;
> +}
> +
> +.tag-line[selected] ~ .tag-line.flash-out {
> +  padding-left: 6px;

We don't want to do this, but actually use the border styles to get the padding. You can see that tag-line (closing tag) is shifting when you switch between parent-child nodes.
Attachment #8807969 - Flags: review?(gl)
Hey Gabriel,

thank you for your fast feedback and helping me get on board, really nice!
Until Friday I'll be at home very late in the evening, so I'll push the changes during the weekend.

Sorry for the delay, greetz 
Marco
(In reply to msfr-develop from comment #16)
> Hey Gabriel,
> 
> thank you for your fast feedback and helping me get on board, really nice!
> Until Friday I'll be at home very late in the evening, so I'll push the
> changes during the weekend.
> 
> Sorry for the delay, greetz 
> Marco

No problem Marco. Thank you for working on this! Feel free to ping me on irc if you need assistance.
Attached patch Bug1241059_prt1.patch (obsolete) — — Splinter Review 
Made all changes proposed in last review (with some little differences):

1. Added a 2px top padding to .children (background-position-y: 2px;) and a 2px bottom padding to the closing element (background-position-y: -2px;), but didn't change the background-hight (background-size: ...).

2. Because the class .flash-out did only add a 0.5sec transition to the application of background properties and I could not notice any differences if this class was omitted, so I removed it.
Attachment #8810209 - Flags: feedback?(gl)
Comment on attachment 8810209 [details] [diff] [review]
Bug1241059_prt1.patch

# HG changeset patch
# User Marco Schaefer <msfr-develop@posteo.me>
# Date 1479063327 -3600
#      Sun Nov 13 19:55:27 2016 +0100
# Node ID 48d33396e7cd9f1da76b3db19b44445c59dc41c7
# Parent  c44c01dfd264370c1558b747525d220a9a89b51c
Bug 1241059 - Part 1: Extend the markup outline to the closing tag r=gl

diff --git a/devtools/client/themes/markup.css b/devtools/client/themes/markup.css
--- a/devtools/client/themes/markup.css
+++ b/devtools/client/themes/markup.css
@@ -139,17 +139,21 @@ ul.children + .tag-line::before {
   padding-left: 2px;
 }
 
-.tag-line[selected] + .children {
+.tag-line[selected] + .children,
+.tag-line[selected] ~ .tag-line {
   background-image: linear-gradient(to top, var(--markup-outline), var(--markup-outline));
+  background-position-x: -6px;
   background-repeat: no-repeat;
-  /* Shorten the outline height by 4px to account for the 2px top padding and
-   * allow for a 2px bottom padding */
-  background-size: 1.5px calc(100% - 4px);
-  /* Align the outline to under the expander arrow and provide 2px top
-   * padding */
-  background-position: -6px 2px;
+  background-size: 1.5px 100%;
   border-left: 6px solid transparent;
-  margin-left: -6px;
+}
+
+.tag-line[selected] + .children {
+  background-position-y: 2px;
+}
+
+.tag-line[selected] ~ .tag-line {
+  background-position-y: -2px;
 }
 
 .html-editor-container {
@@ -223,10 +227,6 @@ ul.children + .tag-line::before {
   margin-right: 0;
 }
 
-.flash-out {
-  transition: background .5s;
-}
-
 .markupview-events {
   display: none;
   cursor: pointer;
Attachment #8810209 - Attachment is obsolete: true
Attachment #8810209 - Flags: feedback?(gl)
Attached patch Bug1241059_prt1.patch (obsolete) — — Splinter Review 
Made all changes proposed in last review (with some little differences):

1. Added a 2px top padding to .children (background-position-y: 2px;) and a 2px bottom padding to the closing element (background-position-y: -2px;), but didn't change the background-hight (background-size: ...).

2. Because the class .flash-out did only add a 0.5sec transition to the application of background properties and I could not notice any differences if this class was omitted, so I removed it.
Attachment #8810212 - Flags: feedback?(gl)
Attachment #8807969 - Attachment is obsolete: true
Comment on attachment 8810212 [details] [diff] [review]
Bug1241059_prt1.patch

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

Hi Marco, 

Good job, the CSS changes are exactly what I had in mind for the markup outline. I have added some feedback on what we need to do next. Please let me know if you need any assistance at any time. Again thank you for working on this.

Cheer,
Gabriel

::: devtools/client/themes/markup.css
@@ -151,2 @@
>    border-left: 6px solid transparent;
> -  margin-left: -6px;

We need to keep the margin-left: -6px otherwise, we will see the children shifting.

@@ -223,4 @@
>    margin-right: 0;
>  }
>  
> -.flash-out {

We need to keep .flash-out unfortunately. If you go to http://www.timer-tab.com and inspect #counter. You will see that the flash-out is used for the orange background when the node changes. We should try to keep this behavior. What I think we need to do is basically remove the flash-out class on the tag-line when it is selected, and re-add when we select somewhere else.
Attachment #8810212 - Flags: feedback?(gl) → feedback+
- Put back the .flash-out class.
- Put back margin-left: -6px.
- If .tag-line[selected] ~ .tag-line class triggers, the transition will be unset by setting transition-property: none. 

Greetings, Marco
Attachment #8812175 - Flags: feedback?(gl)
Attachment #8810212 - Attachment is obsolete: true
Comment on attachment 8812175 [details] [diff] [review]
Bug1241059_prt1.patch

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

::: devtools/client/themes/markup.css
@@ +226,5 @@
>    text-decoration: underline;
>  }
>  
> +.flash-out {
> +  transition: background .5s;

Let's keep this rule in the same position.
Attachment #8812175 - Flags: review+
Attachment #8812175 - Flags: feedback?(gl)
Hi Marco,

I went ahead and landed your part 1 patch. Thank you for your hard work and congrats on getting your first patch into Firefox! As always you can message us on the channel if you need any help on the next part of the bug. 

Cheers,
Gabriel
Hi Gabriel, 

these are great news an I'm super happy :) What about the other parts of this bug? I can start on h the next part at the weekend (it's the old part 1 of the bug).

"when clicking on a closing tag, this should not scroll back up to the corresponding opening tag. Instead, it should mark the closing tag as selected"
https://hg.mozilla.org/mozilla-central/rev/1f2ff51b2169
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
For the next part, I think you should look into how the markup select works, and disable scrolling the node into view in the markup view.
Whiteboard: [btpp-fix-later] → [btpp-fix-later][todo-mr]
Hi Marco,

Are you still working on this? Otherwise I could work on part 2.
Flags: needinfo?(msfr-develop)
Hi Matteo,

unfortunatly I actually haven't the time to work on it. It would be great if you'll fix the bug. 

Thank you for asking. Happy coding,
Marco
Flags: needinfo?(msfr-develop)
Whiteboard: [btpp-fix-later][todo-mr] → [btpp-fix-later]
Hi, 
is this bug still open? I would like to work on it if it still hasn't been fixed. 

Tim
(In reply to Patrick Brosset <:pbro> from comment #7)
> Let's get this moving.
> There are 2 things to fix here:
> 
> 1) when clicking on a closing tag, this should not scroll back up to the
> corresponding opening tag. Instead, it should mark the closing tag as
> selected
> 
> 2) the vertical line should extend all the way down to the closing tag
> http://searchfox.org/mozilla-central/source/devtools/client/themes/markup.
> css#142
> 
> 3) when 1) is fixed, I believe this would go away.

Seems like I can still reproduce 1). I will start working on this ASAP, please let me know if that's possible. 

Thank You
Closing tag does not seem like an individual component,but rather a part of the opening tag's container. So selecting the closing tag will select the whole container and which highlights the opening tag instead. It seems like fixing this could modify quite a bit of the markup-container.js.
Flags: needinfo?(gl)
Yes, this will involve MarkupContainer. I am not quite sure if we want to fix this since the markup view is also slowly being rewritten.
Flags: needinfo?(gl)
Product: Firefox → DevTools
The leave-open keyword is there and there is no activity for 6 months.
:gl, maybe it's time to close this bug?
Flags: needinfo?(gl)

Hey,
I am Looking for the good first bug as I am an outreachy applicants. I can take up this issue.
Thanks,
Dhruvi

Hello! I am applying for this Outreachy round and I am looking for bugs/issues I could help with to learn as much as possible. Can I still help with this bug? Or maybe could you assign me another good-first-bug I could work with?

I will file a new bug for this. There were several things to fix here as discussed in previous comments. And someone has already fixed one of the parts (Marco), in Firefox 53.
This bug was left open back then, in the hope that the second part would also be fixed shortly after. It didn't. So it's better to just mark this one as fixed, and file another bug for the remaining problem.

So, please head over to bug 1530270 for the other part.

Status: REOPENED → RESOLVED
Closed: 8 years ago → 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Whiteboard: [btpp-fix-later]
Assignee: nobody → msfr-develop

Clearing needinfo since this has been resolved.

Flags: needinfo?(gl)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: