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

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Brian Grinstead [:bgrins]]

Attachment: 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

[Paul Rouget [:paul]]

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

Comment 2

[Brian Grinstead [:bgrins]]

(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.

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: 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.

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: 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;
}

Comment 5

[Brian Grinstead [:bgrins]]

Attachment: 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).

Comment 6

[Paul Rouget [:paul]]

(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?

Comment 7

[Brian Grinstead [:bgrins]]

(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.

Comment 8

[Brian Grinstead [:bgrins]]

Can you please land this when you get a chance?

Comment 9

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/7ecc377b80ab

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7ecc377b80ab