970962 - [markup view] cursor goes caret when over inter-attribute and node-attribute spaces

Status: RESOLVED FIXED

Description

[Joe Walker [:jwalker] (needinfo me or ping on irc)]

STR:

1. Find an element with attributes in the markup view.
2. Mouseover the open tag
3. Move your mouse right until it's over the space after the tag name

Expected results:

* The cursor remains as a pointer because nothing different happens when you click

Actual results

* Cursor shows a caret

Comment 1

[vikneshwar]

Hai Patrick , 
   Can you help me with this :)

Comment 2

[Patrick Brosset <:pbro>]

(In reply to vikneshwar from comment #1)
> Hai Patrick , 
>    Can you help me with this :)

This should be a simple CSS fix. There are 2 main CSS files for the markup-view: one that controls theme-related things (colors), and the other one that controls the general layout of the panel.
I think this fix should go in the latter one, because this isn't going to be different depending on the selected theme.

This file is: browser/devtools/markupview/markup-view.css

In this file, we need to make sure the 'default' cursor is used in the markup-view.
I think something like this should work `.editor .open, .editor .close {cursor: default;}`

Comment 3

[vikneshwar]

Created attachment 8420335 [details] [diff] [review]
cursor-markupview.patch

Comment 4

[Patrick Brosset <:pbro>]

Comment on attachment 8420335 [details] [diff] [review]
cursor-markupview.patch

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

Please test your patch before submitting it. This one doesn't work.

::: browser/themes/shared/devtools/markup-view.css
@@ +32,5 @@
>  
>  .tag-line {
>    padding-left: 2px;
>  }
> +.editor .open , .editor . close 

3 formatting details:
- remove the trailing whitespace,
- put the rule opening brace '{' on the line of the selector, like all the other rules,
- add an empty line in between all rules

1 syntax error:
- there shouldn't be a space in '. close', this will cause the selector to be invalid and it won't apply.
This means you're not using a text editor with proper syntax highlighting, otherwise you'd have seen it.
Also, this means you haven't tested this patch at all, you'd have seen that the bug wasn't fixed.

Any patch you submit should be thoroughly tested, at the very least manually, but just building again and checking yourself if things look good. And normally, automated non-regression tests should also be written, although in this case I don't think that's required.

Comment 5

[vikneshwar]

Created attachment 8420929 [details] [diff] [review]
cursor-markup-view.patch

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8420929 [details] [diff] [review]
cursor-markup-view.patch

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

Much better :) thanks.
I noticed one bug though:
the cursor is fine as long as the node is collapsed, but if you expand a node and hover over the close tag, then the cursor is a caret where it should be default instead.

Comment 7

[Patrick Brosset <:pbro>]

I think '.tag-line .open, .tag-line .close' is more appropriate for the selector.
Also, less important but the cursor also goes caret over comment nodes. It should be easy to fix it while we're at it in this bug. You can check markup-view.xul to see what classes comment nodes have on them.

Comment 8

[vikneshwar]

Created attachment 8421199 [details] [diff] [review]
default-cursor-markupview.patch

Added CSS properties to use default cursor for comment node and close tags 
Please , tell me if any more changes are needed . Thank you :)

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8421199 [details] [diff] [review]
default-cursor-markupview.patch

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

As said in comment 4, it looks like you haven't tested this patch much. If after adding '#template-comment {...}' in the code you had tested, you'd have seen that it had no effect, and would have therefore removed it from the patch. The same applies for '.comment' and '.theme-comment', you'd have seen that adding both had the same effect as just adding one.

Please test your changes! There's no point asking for F? or R? for something you haven't tested and are not sure about.

::: browser/devtools/markupview/markup-view.css
@@ +165,5 @@
>    background: rgba(205,205,255,0.2);
>    outline: 1px solid transparent;
>  }
> +
> +.editor .open, .editor .close {

Why did you keep this selector, you already have '.tag-line .open, .tag-line .close' further below which is what you need to use. This rule isn't needed.

@@ +173,5 @@
> +.comment {
> +  cursor: default;
> +}
> +
> +#template-comment {

There's no element with ID template-comment that is inserted in the markup-view. This rule isn't needed.

@@ +177,5 @@
> +#template-comment {
> +  cursor: default;
> +}
> +
> +.theme-comment {

You already have '.comment' which is enough to cover comment nodes, no need for '.theme-comment' here.

@@ +181,5 @@
> +.theme-comment {
> +  cursor: default;
> +}
> +
> +.tag-line .open, .tag-line .close {

Since all these rules are for doing the same thing, please group all the selectors in the same rule, separated by commas:

.tag-line .open, .tag-line .close, .comment {
  cursor: default;
}

that's 3 lines instead of the 15 lines you added.

Comment 10

[vikneshwar]

Created attachment 8421218 [details] [diff] [review]
cursor.patch

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8421218 [details] [diff] [review]
cursor.patch

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

LGTM

Comment 12

[Patrick Brosset <:pbro>]

Created attachment 8421249 [details] [diff] [review]
cursor.patch

Fixing the commit message and marking all other patches as obsolete.

Comment 13

[Carsten Book [:Tomcat]]

Hi,
could you provide a Try link. Suggestions for what to run if you haven't
yet can be found here:
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Comment 14

[Patrick Brosset <:pbro>]

Sorry about that. I completely forgot. Probably because the patch was only CSS related, I didn't think of it.
Here it is: https://tbpl.mozilla.org/?tree=Try&rev=a7526363a529

Comment 15

[Patrick Brosset <:pbro>]

Try looks green.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/557bf7ef4fc4

Comment 17

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/557bf7ef4fc4