Markup View - Selected nodes with word wrapped attributes are not fully highlighted

RESOLVED FIXED in Firefox 26

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

unspecified
Firefox 26
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Assignee)

Description

5 years ago
Created attachment 789812 [details]
markupview-wrapping-highlight.png

The theme-selected highlight seems to skip any middle lines.  See screenshot, and notice the white background between the grey rows.

Comment 1

5 years ago
My guess: it's because the span are a mix of inline and inline-block. Everything should be inline-block (I haven't verified).
(Assignee)

Comment 2

5 years ago
(In reply to Paul Rouget [:paul] from comment #1)
> My guess: it's because the span are a mix of inline and inline-block.
> Everything should be inline-block (I haven't verified).

OK, I will take a look at it.
Assignee: nobody → bgrinstead
(Assignee)

Comment 3

5 years ago
Created attachment 790247 [details]
Markup View when removing inline-block rule on span

The rule causing this issue is in themes/shared/devtools/markup-view.css:


/* Give some padding to focusable elements to match the editor input
 * that will replace them.
 */
span[tabindex] {
  display: inline-block;
  padding: 1px 0;
}

If we completely remove the inline-block rule, a few things happen:

1) The background works as expected
2) The outline now wraps (see the image on the right, there are two separate outlines)
3) The `src=` portion of the attribute stays on the first line, rather than dropping to the second.

From my point of view, 1 and 3 are both positives.  Not sure about 2.  Another alternative would be to set the background on the entire element rather than just the text.  I will follow up with more information on that.
(Assignee)

Comment 4

5 years ago
Created attachment 790250 [details]
markupview-block-highlight.png

Actually, doing a highlight on the whole tagname block is going to be trickier than I thought, but it would look something like this, and the general CSS is something like this:


.editor {
  display: block;
}
.expander {
  position: absolute;
}
(Assignee)

Comment 5

5 years ago
Created attachment 790266 [details] [diff] [review]
markupview-904842.patch

Given all of these options, I like just allowing the span to be inline the most.  Paul, do you think the right side of the image here: https://bug904842.bugzilla.mozilla.org/attachment.cgi?id=790247 is OK as far as UI?  

This patch also removes the padding because otherwise the padding gets overlapped on multiple rows.  It adds the padding back just on the newattr span (the span that shows up when you click near the end of the tag name) since it needs the extra height to match up with the editable field (the attributes with text in them seem the same height-wise).
Attachment #790266 - Flags: feedback?(paul)

Comment 6

5 years ago
(In reply to Brian Grinstead [:bgrins] from comment #5)
> Created attachment 790266 [details] [diff] [review]
> markupview-904842.patch
> 
> Given all of these options, I like just allowing the span to be inline the
> most.  Paul, do you think the right side of the image here:
> https://bug904842.bugzilla.mozilla.org/attachment.cgi?id=790247 is OK as far
> as UI?  

Yes. Can we wrap anywhere? (here, it wraps at the dash).

> This patch also removes the padding because otherwise the padding gets
> overlapped on multiple rows. It adds the padding back just on the newattr
> span (the span that shows up when you click near the end of the tag name)
> since it needs the extra height to match up with the editable field (the
> attributes with text in them seem the same height-wise).

How does it feel when the editable field shows up?
(Assignee)

Comment 7

5 years ago
(In reply to Paul Rouget [:paul] from comment #6)

> Yes. Can we wrap anywhere? (here, it wraps at the dash).

Probably.  But I'm not sure that is a bad thing for it to break naturally.  It is tricky to get `word-wrap: break-word` or `word-break: break-all` to apply without messing up the layout.  Also, there is some discussion about this over at Bug 893677 - many attributes will either have plenty of breaking characters, or will end up being collapsed by that item anyway so I'm not sure that special wrapping needs to be done here.

> How does it feel when the editable field shows up?

It feels fine to me.  Besides the highlighting working, and the multiple outlines, there is a difference regarding where the attribute name shows up when viewing (once editing it is about the same, which causes a jump to the new location with this patch).  If the inplace editor allowed word wrapping in the future this may not be an issue, but that is a different item altogether.  See this quick screencast for a comparison to current release version of FF (the version with the patch applied is on the right): http://www.screencast.com/t/466iIg91MjY.

Updated

5 years ago
Attachment #790266 - Flags: feedback?(paul) → review+
(Assignee)

Comment 8

5 years ago
Can you please land this when you get a chance?
Flags: needinfo?(paul)
Whiteboard: [land-in-fx-team]

Comment 9

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/7ecc377b80ab
Flags: needinfo?(paul)
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/7ecc377b80ab
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.