Status

defect
P1
normal
VERIFIED FIXED
5 years ago
Last year

People

(Reporter: gl, Assigned: gl)

Tracking

unspecified
Firefox 40
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox40 verified)

Details

(Whiteboard: [polish-backlog][difficulty=hard])

Attachments

(11 attachments, 38 obsolete attachments)

127.16 KB, image/png
Details
62.91 KB, image/png
Details
18.42 KB, image/png
Details
994 bytes, image/svg+xml
Details
1000 bytes, image/svg+xml
Details
998 bytes, image/svg+xml
Details
34.20 KB, patch
Details | Diff | Splinter Review
24.71 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
19.23 KB, patch
Details | Diff | Splinter Review
2.33 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
23.69 KB, patch
Details | Diff | Splinter Review
Assignee

Description

5 years ago
Posted patch ruleview-part-1-search.patch (obsolete) — Splinter Review
No description provided.
Assignee

Updated

5 years ago
Assignee: nobody → gabriel.luong
This would be nice
Assignee

Comment 2

5 years ago
Posted patch ruleview-part-1-search.patch (obsolete) — Splinter Review
Attachment #8547760 - Attachment is obsolete: true
Attachment #8555955 - Flags: review?(bgrinstead)
Comment on attachment 8555955 [details] [diff] [review]
ruleview-part-1-search.patch

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

I like it.  I feel like there should be a 'clear filter' button at the bottom of the rule view to help make it clear where there are no (or very few) results because of a filter after switching to a element.  Before fully reviewing this, I'm going to flag a ux-review for UX feedback about this change in the rule view.

Stephen, can you take a look at this build (or point us to someone who can)?  Basically, this adds a filter textbox to the top of the rule view so you could type 'background' and remove rules that don't include that string in a declaration or selector.  It keeps the entire rule visible if any declaration matches, and it highlights that declaration line with a background color.  It's similar to the current filtering in the computed view (in fact, it moves that filter box to the top of the computed view to match this UI).  Do you think this is the best way to surface filtering behavior in the UI?  Here is a try push with binaries: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3c88f59b3f8

::: browser/devtools/styleinspector/cssruleview.xhtml
@@ +51,5 @@
> +      <!--
> +      templateRoot sits at the top of the window. For data it needs an instance of CssRuleView.
> +      -->
> +      <div id="templateRoot">
> +        <xul:hbox class="devtools-toolbar" flex="1" align="center">

I'm somehow able to tab into this (or some container) element after tabbing out of the textbox.  For example: Focus the textbox, press tab.  Or focus the last rule then shift+tab.  We should make sure it doesn't receive focus

@@ +53,5 @@
> +      -->
> +      <div id="templateRoot">
> +        <xul:hbox class="devtools-toolbar" flex="1" align="center">
> +          <xul:textbox class="devtools-searchinput" type="search" save="${searchField}"
> +                       placeholder="&userStylesSearch;" flex="1"

This string should probably say "Filter" or "Filter Styles" (both here and in the computed view)

::: browser/devtools/styleinspector/rule-view.js
@@ +1551,5 @@
>      this._prefObserver.off(PREF_UA_STYLES, this._handlePrefChange);
>      this._prefObserver.off(PREF_DEFAULT_COLOR_UNIT, this._handlePrefChange);
>      this._prefObserver.destroy();
>  
> +    this.searchField.removeEventListener("command", this.filterChanged);

This handler is bound with DOM0 oncommand - does this really need to be removed?  Actually, I wonder if we actually need any of the templating code in this case.  Could we instead just put the textbox / toolbar in the html, give the textbox an ID and do:

this.searchField = doc.querySelector("#foo");
this.searchField.addEventListener("command", this.filterChanged);

Then we could still unbind here and it seems like that would lower the amount of code used  in this file for all the templating.  Or is there some other benefit that we are getting by using templating here?

::: browser/themes/shared/devtools/ruleview.css
@@ +217,5 @@
>  
> +.theme-light .ruleview-property[selected],
> +.theme-light .ruleview-selector[selected],
> +.theme-light .ruleview-selector > span[selected] {
> +  background-color: #dcecf4;

Just use var(--theme-selection-background-semitransparent) for these selectors regardless of theme.  If it doesn't look good with the light theme, we should re-evaluate the color used for that variable.
Attachment #8555955 - Flags: ui-review?(shorlander)
Attachment #8555955 - Flags: review?(bgrinstead)
Attachment #8555955 - Flags: feedback+
Assignee

Comment 4

5 years ago
Posted patch ruleview-part-1-search.patch (obsolete) — Splinter Review
Attachment #8555955 - Attachment is obsolete: true
Attachment #8555955 - Flags: ui-review?(shorlander)
Attachment #8556905 - Flags: ui-review?(shorlander)
Assignee

Comment 5

5 years ago
Posted patch ruleview-part-1-search.patch (obsolete) — Splinter Review
Attachment #8556905 - Attachment is obsolete: true
Attachment #8556905 - Flags: ui-review?(shorlander)
Attachment #8556906 - Flags: ui-review?(shorlander)
Currently evaluating this, bug 987365 and bug 1120406 together since they are mostly linked.

Darren had a preliminary mockup for the class lock (https://bug938202.bugzilla.mozilla.org/attachment.cgi?id=831594) but I asked him and he didn't have anything beyond that.

Updated

5 years ago
Duplicate of this bug: 1061073
Posted image Rules Filter – 01
> This string should probably say "Filter" or "Filter Styles" (both here and
> in the computed view)

I like "Filter Styles". It's consistent with what we do in Console and it's not really a full search.

> I like it.  I feel like there should be a 'clear filter' button at the
> bottom of the rule view to help make it clear where there are no (or very
> few) results because of a filter after switching to a element.  Before fully
> reviewing this, I'm going to flag a ux-review for UX feedback about this
> change in the rule view.

I think if we highlight the Filter field to match the highlighted results and put a clear (X) in the field then that should be enough to call attention to the fact that you are viewing filtered results. An alternative that is less minimalistic but more informative would be to show a bar telling you how how many results you are missing with a button to clear the filters. I first option.

The only other suggestion I have is to use a stronger color than blue for the highlight. I think a bright orange would make it more obvious which results are being filtered.
Comment on attachment 8556906 [details] [diff] [review]
ruleview-part-1-search.patch

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

See comment 8.
Attachment #8556906 - Flags: ui-review?(shorlander) → ui-review-
Assignee

Comment 10

4 years ago
Updated with the code feedback fixes and some of the UX changes. Still waiting on the dark theme search box error representation for no search results, and "x" button.
Attachment #8556906 - Attachment is obsolete: true
Assignee

Comment 11

4 years ago
Posted image Rules Filter - 02
Chose a color close to the one we are using in the Selector filter but made a little less washed out:

Border: #cc3d3d
Background: #3c2124
Highlight colors for light theme:
Border: #ffbf00
Background: #ffee99
Assignee

Comment 15

4 years ago
Attachment #8562596 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8563990 - Flags: review?(bgrinstead)
Assignee

Updated

4 years ago
Attachment #8563991 - Flags: review?(bgrinstead)
Assignee

Comment 17

4 years ago
I still need the official error state colours for the light theme and highlight colours for the dark theme, but I think we can go ahead and review the patch.
Flags: needinfo?(shorlander)

Comment 19

4 years ago
I just tested this on Windows, looks great ! One issue though, there's an extra search icon at the end of the searchbox :  http://images.devs-on.net/Image/3MDZyhnLT0uMY0Pu-Region.png
(In reply to Stephen Horlander [:shorlander] from comment #13)
> Created attachment 8563189 [details]
> search-clear-icons-01.zip
> 
> Clear the search/filter field assets.

What do you think about using SVG for these icons?  It would simplify the CSS and make 2x support easier.

I also notice that each icon has a *-small.png variation in the zip that aren't used in the patch - I'm not sure what the intention of those are, but this could probably be handled with a single SVG as well (which would mean we just need 3 icons instead of 12).
Comment on attachment 8563990 [details] [diff] [review]
Part 1: Implement filter styles in rule view

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

Works great overall - left some notes in comments

::: browser/devtools/styleinspector/rule-view.js
@@ +1347,5 @@
> +  _onContextMenu: function(event) {
> +    try {
> +      // In the sidebar we do not have this.doc.popupNode so we need to save
> +      // the node ourselves.
> +      this.doc.popupNode = event.explicitOriginalTarget;

What's the problem if we have this handler attached to this.element instead of this.document?  Anyway, when right clicking in the textbox I see

TypeError: document.commandDispatcher is undefined textbox.xml:487:20

@@ +1511,5 @@
> +    if (this._filterChangedTimeout) {
> +      clearTimeout(this._filterChangedTimeout);
> +    }
> +
> +    this._filterChangedTimeout = setTimeout(() => {

When I press 'X' it takes 300ms before it clears.  I'd prefer if it was instant in this case.  Could it be set to a 0 ms timeout if the value is empty?

@@ +1516,5 @@
> +      if (this.searchField.value.length > 0) {
> +        this.searchField.setAttribute("filled", true);
> +      } else {
> +        this.searchField.removeAttribute("filled");
> +        this.searchField.classList.remove("devtools-style-searchbox-no-match");

Why remove the class here? It gets removed in createEditors if necessary

@@ +1522,5 @@
> +
> +      this._clearRules();
> +      this._createEditors();
> +
> +      let evt = this.doc.createEvent("Events");

What about:

this.inspector.emit("rule-view-filtered")

And then in the test we can do inspector.once("rule-view-filtered");

@@ +1844,5 @@
> +        rule.editor = new RuleEditor(this, rule);
> +      }
> +
> +      // Handle filter styles if there is a search input
> +      if (searchTerm && rule.domRule.type !== ELEMENT_STYLE) {

I wonder if we should include element styles in the filtering.  If there was a case where an element had a bunch of inline styles it may be annoying if you can't get rid of them.

@@ +1847,5 @@
> +      // Handle filter styles if there is a search input
> +      if (searchTerm && rule.domRule.type !== ELEMENT_STYLE) {
> +        matchSearchTerm = this.highlightRules(rule, searchTerm);
> +
> +        if (matchSearchTerm && !seenSearchTerm) {

I don't see any reason that we need !seenSearchTerm in the condition here.  Seems like this would be equivalent for our case:

if (matchSearchTerm) {
  seenSearchTerm = true;
} else {
  continue;
}

@@ +1891,5 @@
>          this.element.appendChild(rule.editor.element);
>        }
>      }
> +
> +    if (searchTerm && !seenSearchTerm) {

This does highlight a case where it didn't actually filter anything - which is an element with no styles applied and any search term.  In this case it shows up as red, but nothing actually got filtered.

An alternative could be to track whether all rules were filtered and set the no-match class in that case.  But it's not too big of a deal.

@@ +1912,5 @@
> +  highlightRules: function(aRule, aValue) {
> +    let isHighlighted = false;
> +
> +    // Highlight search matches in the rule selectors
> +    if (aRule.domRule.type === Ci.nsIDOMCSSRule.KEYFRAME_RULE &&

Nit: to remove duplication here, maybe do something like this: 

let selectorNodes = [...aRule.editor.selectorText.childNodes];
if (aRule.domRule.type === Ci.nsIDOMCSSRule.KEYFRAME_RULE) {
  selectorNodes = [aRule.editor.selectorText];
}

for (let node of selectorNodes) {
  if (node.textContent.toLowerCase().indexOf(aValue) != -1) {
    node.setAttribute("selected", true);
    this._highlightedElements.push(node);
    isHighlighted = true;
  } else {
    node.removeAttribute("selected");
  }
}

::: browser/themes/shared/devtools/toolbars.inc.css
@@ +342,5 @@
> +  background-position: calc(100% - 8px) center;
> +}
> +
> +.theme-dark .devtools-style-searchbox[filled] {
> +  background-color: #483d22;

I'd most prefer that we use existing theme colors.  But if that won't work for the UX, I'd prefer to set variables on .theme-light and .theme-dark and then used them unconditionally for each individual rule.  For instance:

.theme-dark {
  --searchbox-background-color: #483d22;
}
.theme-light {
  --searchbox-background-color: #ffee99;
}
.devtools-style-searchbox[filled] {
  background-color: var(--searchbox-background-color);
  ....
}

@@ +351,5 @@
> +  background-color: #ffee99;
> +  border-color: #ffbf00;
> +}
> +
> +.theme-dark .devtools-searchinput > .textbox-input-box > .textbox-search-icons > .textbox-search-clear {

I hope this clear icon styling can be cleared up if we switch to svg for the icon

::: browser/themes/windows/jar.mn
@@ +431,5 @@
>          skin/classic/browser/devtools/app-manager/rocket.svg                (../shared/devtools/app-manager/images/rocket.svg)
>          skin/classic/browser/devtools/app-manager/noise.png                 (../shared/devtools/app-manager/images/noise.png)
>          skin/classic/browser/devtools/app-manager/default-app-icon.png      (../shared/devtools/app-manager/images/default-app-icon.png)
> +        skin/classic/browser/devtools/search-clear-failed-small.png         (../shared/devtools/images/search-clear-failed-small.png)
> +        skin/classic/browser/devtools/search-clear-failed-small@2x.png      (../shared/devtools/images/search-clear-failed-small@2x.png)

These -small icons are added in the jar but never used in the patch
Attachment #8563990 - Flags: review?(bgrinstead) → feedback+
Status: NEW → ASSIGNED
Comment on attachment 8563991 [details] [diff] [review]
Part 2: Adjust the styles in the computed view's filter style

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

LGTM

::: browser/devtools/styleinspector/computed-view.js
@@ +534,3 @@
>        this.refreshPanel();
>        this._filterChangeTimeout = null;
>      }, FILTER_CHANGED_TIMEOUT);

Same comment about how I wish the filter would be cleared quicker if the value has been cleared (in Comment 21)
Attachment #8563991 - Flags: review?(bgrinstead) → review+
Posted image search-clear.svg
SVG instead of PNG.
Attachment #8563189 - Attachment is obsolete: true
Flags: needinfo?(shorlander)
- Colors — Light Theme -
Filter Field — Failed:
Border: #e52e2e
Background: #ffe5e5

Filter Field — Highlight:
Border: #ffbf00
Background: #ffee99

Rule View — Highlight:
Background: #ffee99


- Colors — Dark Theme -
Filter Field — Failed:
Border: #cc3d3d
Background: #402325

Filter Field — Highlight:
Border: #d99f2b
Background: #4d4222

Rule View — Highlight:
Background: #594724

Updated

4 years ago
Blocks: 1136101
Assignee

Comment 27

4 years ago
Comment on attachment 8563990 [details] [diff] [review]
Part 1: Implement filter styles in rule view

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

::: browser/devtools/styleinspector/rule-view.js
@@ +1347,5 @@
> +  _onContextMenu: function(event) {
> +    try {
> +      // In the sidebar we do not have this.doc.popupNode so we need to save
> +      // the node ourselves.
> +      this.doc.popupNode = event.explicitOriginalTarget;

I believe the TypeError is because cssruleview.xhtml is not .xul in which there would be a window object. According to https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/Focus_and_Selection, the currently focused element is held by an object called a command dispatcher, of which there is only one for the window.

I opted to use this.element in the revised patch, but my main problem is that now right clicking on the textbox does not provide any context menu for the normal textbox functionality such as copy and paste. To contrast, you can right click on the textbox for inspector search or debugger search.

The TypeError currently exist in both the ruleview and computedview textbox.

I believe we should at the very least have the usual textbox context menu, but I think that will involve changing cssruleview and computedview to .xul. Any thoughts on this?

@@ +1511,5 @@
> +    if (this._filterChangedTimeout) {
> +      clearTimeout(this._filterChangedTimeout);
> +    }
> +
> +    this._filterChangedTimeout = setTimeout(() => {

Fixed.

@@ +1516,5 @@
> +      if (this.searchField.value.length > 0) {
> +        this.searchField.setAttribute("filled", true);
> +      } else {
> +        this.searchField.removeAttribute("filled");
> +        this.searchField.classList.remove("devtools-style-searchbox-no-match");

The idea was to try to get the classes removed as quickly as possible when I press 'X', but changing the filter timeout from 300ms to 0ms made this unnecessary.

@@ +1522,5 @@
> +
> +      this._clearRules();
> +      this._createEditors();
> +
> +      let evt = this.doc.createEvent("Events");

Fixed.

@@ +1844,5 @@
> +        rule.editor = new RuleEditor(this, rule);
> +      }
> +
> +      // Handle filter styles if there is a search input
> +      if (searchTerm && rule.domRule.type !== ELEMENT_STYLE) {

I mainly copied chrome's behaviour on including the element styles. I think it is better to include the element style in the event no rules are matched instead of displaying an empty rule view container.

@@ +1847,5 @@
> +      // Handle filter styles if there is a search input
> +      if (searchTerm && rule.domRule.type !== ELEMENT_STYLE) {
> +        matchSearchTerm = this.highlightRules(rule, searchTerm);
> +
> +        if (matchSearchTerm && !seenSearchTerm) {

Fixed.

@@ +1891,5 @@
>          this.element.appendChild(rule.editor.element);
>        }
>      }
> +
> +    if (searchTerm && !seenSearchTerm) {

I think we'll leave it as is.

@@ +1912,5 @@
> +  highlightRules: function(aRule, aValue) {
> +    let isHighlighted = false;
> +
> +    // Highlight search matches in the rule selectors
> +    if (aRule.domRule.type === Ci.nsIDOMCSSRule.KEYFRAME_RULE &&

Fixed.

::: browser/themes/shared/devtools/toolbars.inc.css
@@ +342,5 @@
> +  background-position: calc(100% - 8px) center;
> +}
> +
> +.theme-dark .devtools-style-searchbox[filled] {
> +  background-color: #483d22;

Fixed.

@@ +351,5 @@
> +  background-color: #ffee99;
> +  border-color: #ffbf00;
> +}
> +
> +.theme-dark .devtools-searchinput > .textbox-input-box > .textbox-search-icons > .textbox-search-clear {

Fixed

::: browser/themes/windows/jar.mn
@@ +431,5 @@
>          skin/classic/browser/devtools/app-manager/rocket.svg                (../shared/devtools/app-manager/images/rocket.svg)
>          skin/classic/browser/devtools/app-manager/noise.png                 (../shared/devtools/app-manager/images/noise.png)
>          skin/classic/browser/devtools/app-manager/default-app-icon.png      (../shared/devtools/app-manager/images/default-app-icon.png)
> +        skin/classic/browser/devtools/search-clear-failed-small.png         (../shared/devtools/images/search-clear-failed-small.png)
> +        skin/classic/browser/devtools/search-clear-failed-small@2x.png      (../shared/devtools/images/search-clear-failed-small@2x.png)

Fixed.
Assignee

Comment 28

4 years ago
Attachment #8563990 - Attachment is obsolete: true
Assignee

Comment 29

4 years ago
Attachment #8563991 - Attachment is obsolete: true

Comment 31

4 years ago
Comment on attachment 8571786 [details] [diff] [review]
Part 1: Implement filter styles in rule view

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

A few comments about the svgs (that apply to all of them).
Also, you could try combining them together in one file, like this : http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/content-contextmenu.svg?raw=1

::: browser/themes/shared/devtools/images/search-clear-dark.svg
@@ +1,3 @@
> +<?xml version="1.0" encoding="utf-8"?>
> +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
> +

The doctype and the new line can be removed.

@@ +1,5 @@
> +<?xml version="1.0" encoding="utf-8"?>
> +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
> +
> +<svg version="1.1"
> +     id="icon-filterField-clear"

The ID isn't needed

@@ +5,5 @@
> +     id="icon-filterField-clear"
> +     xmlns="http://www.w3.org/2000/svg"
> +     xmlns:xlink="http://www.w3.org/1999/xlink"
> +     x="0"
> +     y="0"

x and y attributes have no effect on the svg element.

@@ +15,5 @@
> +    <path id="glyphShape-clear" d="M8,0C3.6,0,0,3.6,0,8c0,4.4,3.6,8,8,8s8-3.6,8-8C16,3.6,12.4,0,8,0 z M11.9,10.5l-1.4,1.4L8,9.4l-2.4,2.4l-1.4-1.4L6.6,8L4.2,5.6l1.4-1.4L8,6.6l2.4-2.4l1.4,1.4L9.4,8L11.9,10.5z"/>
> +
> +    <style type="text/css">
> +    <![CDATA[
> +

This blank line and the previous line can be removed.

@@ +19,5 @@
> +
> +      .icon-state-default { fill: #f5f7fa; fill-opacity: .6; }
> +      .icon-state-pressed { fill: #7d7e80; fill-opacity: .8; }
> +
> +    ]]>

Same

::: browser/themes/shared/devtools/toolbars.inc.css
@@ -356,5 @@
> -  background-position: calc(100% - 8px) center;
> -}
> -
> -.devtools-searchinput > .textbox-input-box > .textbox-search-icons {
> -  display: none;

.devtools-searchinput > .textbox-input-box > .textbox-search-icons > .textbox-search-icon still needs to be hidden (it's visible on Windows and looks pretty ugly)
Assignee

Comment 32

4 years ago
(In reply to Tim Nguyen [:ntim] from comment #31)
> Comment on attachment 8571786 [details] [diff] [review]
> Part 1: Implement filter styles in rule view
> 
> Review of attachment 8571786 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A few comments about the svgs (that apply to all of them).
> Also, you could try combining them together in one file, like this :
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/content-
> contextmenu.svg?raw=1
> 
> ::: browser/themes/shared/devtools/images/search-clear-dark.svg
> @@ +1,3 @@
> > +<?xml version="1.0" encoding="utf-8"?>
> > +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
> > +
> 
> The doctype and the new line can be removed.
> 
> @@ +1,5 @@
> > +<?xml version="1.0" encoding="utf-8"?>
> > +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
> > +
> > +<svg version="1.1"
> > +     id="icon-filterField-clear"
> 
> The ID isn't needed
> 
> @@ +5,5 @@
> > +     id="icon-filterField-clear"
> > +     xmlns="http://www.w3.org/2000/svg"
> > +     xmlns:xlink="http://www.w3.org/1999/xlink"
> > +     x="0"
> > +     y="0"
> 
> x and y attributes have no effect on the svg element.
> 
> @@ +15,5 @@
> > +    <path id="glyphShape-clear" d="M8,0C3.6,0,0,3.6,0,8c0,4.4,3.6,8,8,8s8-3.6,8-8C16,3.6,12.4,0,8,0 z M11.9,10.5l-1.4,1.4L8,9.4l-2.4,2.4l-1.4-1.4L6.6,8L4.2,5.6l1.4-1.4L8,6.6l2.4-2.4l1.4,1.4L9.4,8L11.9,10.5z"/>
> > +
> > +    <style type="text/css">
> > +    <![CDATA[
> > +
> 
> This blank line and the previous line can be removed.
> 
> @@ +19,5 @@
> > +
> > +      .icon-state-default { fill: #f5f7fa; fill-opacity: .6; }
> > +      .icon-state-pressed { fill: #7d7e80; fill-opacity: .8; }
> > +
> > +    ]]>
> 
> Same
> 
> ::: browser/themes/shared/devtools/toolbars.inc.css
> @@ -356,5 @@
> > -  background-position: calc(100% - 8px) center;
> > -}
> > -
> > -.devtools-searchinput > .textbox-input-box > .textbox-search-icons {
> > -  display: none;
> 
> .devtools-searchinput > .textbox-input-box > .textbox-search-icons >
> .textbox-search-icon still needs to be hidden (it's visible on Windows and
> looks pretty ugly)

Thanks for the note! I have an updated patch with the search icon hidden ready, but the tree is closed. The current patches are meant as a WIP and for me to get another build to test on linux and window and see any lingering errors. I don't really know too much about svg elements, but I will look into the changes.
Assignee

Comment 33

4 years ago
Attachment #8571786 - Attachment is obsolete: true
Attachment #8572460 - Flags: review?(bgrinstead)
Assignee

Comment 35

4 years ago
Attachment #8571787 - Attachment is obsolete: true
Attachment #8572461 - Attachment is obsolete: true
Attachment #8572461 - Flags: review?(bgrinstead)
Assignee

Updated

4 years ago
Attachment #8572464 - Flags: review?(bgrinstead)
(In reply to Gabriel Luong (:gl) from comment #27)
> Comment on attachment 8563990 [details] [diff] [review]
> Part 1: Implement filter styles in rule view
> 
> Review of attachment 8563990 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/styleinspector/rule-view.js
> @@ +1347,5 @@
> > +  _onContextMenu: function(event) {
> > +    try {
> > +      // In the sidebar we do not have this.doc.popupNode so we need to save
> > +      // the node ourselves.
> > +      this.doc.popupNode = event.explicitOriginalTarget;
> 
> I believe the TypeError is because cssruleview.xhtml is not .xul in which
> there would be a window object. According to
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/
> Focus_and_Selection, the currently focused element is held by an object
> called a command dispatcher, of which there is only one for the window.
> 
> I opted to use this.element in the revised patch, but my main problem is
> that now right clicking on the textbox does not provide any context menu for
> the normal textbox functionality such as copy and paste. To contrast, you
> can right click on the textbox for inspector search or debugger search.
> 
> The TypeError currently exist in both the ruleview and computedview textbox.
> 
> I believe we should at the very least have the usual textbox context menu,
> but I think that will involve changing cssruleview and computedview to .xul.
> Any thoughts on this?

I don't want to convert them to xul if at all possible.  Oddly though, even when I reduce cssruleview.xhtml to a minimal test case no context menu is showing up:

  <?xml version="1.0"?>
  <html xmlns="http://www.w3.org/1999/xhtml"
        xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
    <body>
      Open a context menu: <input id="ruleview-searchbox" />
    </body>
  </html>
Comment on attachment 8572460 [details] [diff] [review]
Part 1: Implement filter styles in rule view

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

This is very good progress. Two things that should be fixed before landing:

1) Typing margin: 8px should keep the correct rules around on the body element at http://bgrins.github.io/devtools-demos/inspector/mutations.html
2) We need to fix the context menu before landing.  I can help with this, but try to see if you can come up with as reduced a test case as possible to demonstrate the problem we are seeing.

::: browser/devtools/styleinspector/rule-view.js
@@ +1511,5 @@
> +    if (this._filterChangedTimeout) {
> +      clearTimeout(this._filterChangedTimeout);
> +    }
> +
> +    let filterTimeout = (this.searchField.value.length > 0) ? 300 : 0;

This feels a little sluggish, can you try updating it to be maybe 150 (here and in computed view)?  And make it a const here just like in computed view.

@@ +1831,5 @@
>      if (!this._elementStyle.rules) {
>        return;
>      }
>  
> +    if (!searchTerm && this._highlightedElements.length > 0) {

Shouldn't it clear the highlight unconditionally here?  Seems like if you typed 'foo' then change it to 'boo' we would want anything that was originally highlighted to be cleared before proceeding.  I see we clear the class later in highlightRules but it'd be nice to see this all done right here instead. (I'm assuming there won't be any flash if we did that since the highlight happens just next, but if that is an issue it would be another story).

@@ +1841,5 @@
>          continue;
>        }
>  
> +      if (!rule.editor) {
> +        rule.editor = new RuleEditor(this, rule);

What's the case when rule.editor is null?  Could use a comment (I know this is some weirdness inherited from below).

@@ +1846,5 @@
> +      }
> +
> +      // Handle filter styles if there is a search input
> +      if (searchTerm && rule.domRule.type !== ELEMENT_STYLE) {
> +        matchSearchTerm = this.highlightRules(rule, searchTerm);

matchSearchTerm can be scoped within this block since it's only used here

@@ +1923,5 @@
> +        selectorNode.setAttribute("selected", true);
> +        this._highlightedElements.push(selectorNode);
> +        isHighlighted = true;
> +      } else {
> +        selectorNode.removeAttribute("selected");

If we call clearHighlight unconditionally above I believe this else can be removed

@@ +1938,5 @@
> +        textProp.editor.element.setAttribute("selected", true);
> +        this._highlightedElements.push(textProp.editor.element);
> +        isHighlighted = true;
> +      } else {
> +        textProp.editor.element.removeAttribute("selected");

If we call clearHighlight unconditionally above I believe this else can be removed

@@ +1942,5 @@
> +        textProp.editor.element.removeAttribute("selected");
> +      }
> +    }
> +
> +    return isHighlighted;

Could just return this._highlightedElements.length > 0 and not keep track of this bit?
Attachment #8572460 - Flags: review?(bgrinstead) → feedback+
Comment on attachment 8572464 [details] [diff] [review]
Part 2: Adjust the styles in the computed view's filter style

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

These changes look fine generally assuming we figure something out for the context menu on the textbox
Attachment #8572464 - Flags: review?(bgrinstead) → review+
(In reply to Brian Grinstead [:bgrins] from comment #38)
> @@ +1831,5 @@
> >      if (!this._elementStyle.rules) {
> >        return;
> >      }
> >  
> > +    if (!searchTerm && this._highlightedElements.length > 0) {
> 
> Shouldn't it clear the highlight unconditionally here?  Seems like if you
> typed 'foo' then change it to 'boo' we would want anything that was
> originally highlighted to be cleared before proceeding.  I see we clear the
> class later in highlightRules but it'd be nice to see this all done right
> here instead. (I'm assuming there won't be any flash if we did that since
> the highlight happens just next, but if that is an issue it would be another
> story).
> @@ +1923,5 @@
> > +        selectorNode.setAttribute("selected", true);
> > +        this._highlightedElements.push(selectorNode);
> > +        isHighlighted = true;
> > +      } else {
> > +        selectorNode.removeAttribute("selected");
> 
> If we call clearHighlight unconditionally above I believe this else can be
> removed
> 
> @@ +1938,5 @@
> > +        textProp.editor.element.setAttribute("selected", true);
> > +        this._highlightedElements.push(textProp.editor.element);
> > +        isHighlighted = true;
> > +      } else {
> > +        textProp.editor.element.removeAttribute("selected");
> 
> If we call clearHighlight unconditionally above I believe this else can be
> removed
> 
> @@ +1942,5 @@
> > +        textProp.editor.element.removeAttribute("selected");
> > +      }
> > +    }
> > +
> > +    return isHighlighted;
> 
> Could just return this._highlightedElements.length > 0 and not keep track of
> this bit?

To be clear, with this clearHighlight() / _highlightedElements stuff there are quite a few ways that it could be solved and I'm not stuck on it being done this way in particular.  The main issue I have with it right now is that the state in _highlightedElements gets out of whack and ends up containing elements that are no longer visible / selected.  We could even just replace that object with a call to querySelectorAll("[selected]"), in which case I wouldn't mind the fact that the attributes are removed inside of the highlightRules function instead of clearHighlight().
Assignee

Comment 41

4 years ago
Comment on attachment 8572460 [details] [diff] [review]
Part 1: Implement filter styles in rule view

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

::: browser/devtools/styleinspector/rule-view.js
@@ +1511,5 @@
> +    if (this._filterChangedTimeout) {
> +      clearTimeout(this._filterChangedTimeout);
> +    }
> +
> +    let filterTimeout = (this.searchField.value.length > 0) ? 300 : 0;

Fixed. Changed to 150 and added const

@@ +1831,5 @@
>      if (!this._elementStyle.rules) {
>        return;
>      }
>  
> +    if (!searchTerm && this._highlightedElements.length > 0) {

I think keeping the second condition should be fine. We only want to clear the highlights if there are highlighted elements.

@@ +1841,5 @@
>          continue;
>        }
>  
> +      if (!rule.editor) {
> +        rule.editor = new RuleEditor(this, rule);

Fixed. This is mainly to initialize the rule editor.

@@ +1846,5 @@
> +      }
> +
> +      // Handle filter styles if there is a search input
> +      if (searchTerm && rule.domRule.type !== ELEMENT_STYLE) {
> +        matchSearchTerm = this.highlightRules(rule, searchTerm);

Fixed

@@ +1923,5 @@
> +        selectorNode.setAttribute("selected", true);
> +        this._highlightedElements.push(selectorNode);
> +        isHighlighted = true;
> +      } else {
> +        selectorNode.removeAttribute("selected");

Fixed

@@ +1938,5 @@
> +        textProp.editor.element.setAttribute("selected", true);
> +        this._highlightedElements.push(textProp.editor.element);
> +        isHighlighted = true;
> +      } else {
> +        textProp.editor.element.removeAttribute("selected");

Fixed

@@ +1942,5 @@
> +        textProp.editor.element.removeAttribute("selected");
> +      }
> +    }
> +
> +    return isHighlighted;

We actually have to keep track of the bit because we need to check for that particular rule if there were any selectors or properties highlighted, and from that it tells us whether or not we should show that rule editor.
Assignee

Comment 42

4 years ago
Attachment #8572460 - Attachment is obsolete: true
Assignee

Comment 43

4 years ago
Attachment #8572464 - Attachment is obsolete: true
Assignee

Comment 44

4 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b93e214f2d5f

Only lingering issue should be the context menu.
Assignee

Comment 45

4 years ago
Attachment #8576540 - Attachment is obsolete: true
Assignee

Comment 46

4 years ago
Comment on attachment 8582893 [details] [diff] [review]
Part 1: Implement filter styles in rule view

Hi Boris,

Pinging you to see if you might have any insight regarding why the input context menu / overlay doesn't display when right clicking. I previously pinged you about the context menu not displaying when we mixed xul and html. I have changed the rule view to pure html in this part 1 patch.

STR:
1. Apply part 1 patch https://bugzilla.mozilla.org/attachment.cgi?id=8582893
2. Open DevTools Inspector
3. Right click on the Filter Styles search input as seen in https://bug1120616.bugzilla.mozilla.org/attachment.cgi?id=8559877
4. No context menu displays

Thanks in advance!
Flags: needinfo?(bzbarsky)
Assignee

Comment 47

4 years ago
(In reply to Gabriel Luong (:gl) from comment #46)
> Comment on attachment 8582893 [details] [diff] [review]
> Part 1: Implement filter styles in rule view
> 
> Hi Boris,
> 
> Pinging you to see if you might have any insight regarding why the input
> context menu / overlay doesn't display when right clicking. I previously
> pinged you about the context menu not displaying when we mixed xul and html.
> I have changed the rule view to pure html in this part 1 patch.
> 
> STR:
> 1. Apply part 1 patch https://bugzilla.mozilla.org/attachment.cgi?id=8582893
> 2. Open DevTools Inspector
> 3. Right click on the Filter Styles search input as seen in
> https://bug1120616.bugzilla.mozilla.org/attachment.cgi?id=8559877
> 4. No context menu displays
> 
> Thanks in advance!

Another easier STR:
1. Go to https://bugzilla.mozilla.org/
2. Open DevTools Inspector and highlight the <body>
3. Select the Fonts tab in the Inspector
4. Right click on the input box for the font url eg) "https://bugzilla.mozilla.org/skins/contrib/Mozilla/fira/FiraSans-Italic.woff"
5. No context menu for cut, copy, paste, etc appears
(In reply to Gabriel Luong (:gl) from comment #47)
> (In reply to Gabriel Luong (:gl) from comment #46)
> > Comment on attachment 8582893 [details] [diff] [review]
> > Part 1: Implement filter styles in rule view
> > 
> > Hi Boris,
> > 
> > Pinging you to see if you might have any insight regarding why the input
> > context menu / overlay doesn't display when right clicking. I previously
> > pinged you about the context menu not displaying when we mixed xul and html.
> > I have changed the rule view to pure html in this part 1 patch.

Here is an even easier STR.  Paste this into the Browser Console and notice that the appended input element doesn't have a context menu:

let nbox = gBrowser.getNotificationBox(gBrowser.selectedBrowser)
let frame = gBrowser.ownerDocument.createElement("iframe");
frame.setAttribute("src", "data:text/html,<body>No context menu: <input /></body>");
nbox.appendChild(frame);
However when running this command with a xul document, the context menu does work:

let nbox = gBrowser.getNotificationBox(gBrowser.selectedBrowser)
let frame = gBrowser.ownerDocument.createElement("iframe");
frame.setAttribute("src",
    "data:application/vnd.mozilla.xul+xml;charset=UTF-8,<?xml version='1.0'?>" +
    "<?xml-stylesheet href='chrome://global/skin/global.css'?>" +
    "<window xmlns='http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul'>" +
    "<textbox></textbox></window>"
);
nbox.appendChild(frame);
> 4. No context menu displays

OK.  So your problem, afaict, is that chrome documents are responsible for their own context menus.

So for example, the only reason you get context menus at all in web content is that the <xul:browser> inside <tabbrowser> has contextmenu="contentAreaContextMenu" set on it which then does the right things.

Or as another example, the only reason you get a context menu when context-clicking in the inspector tree view is that this lives in a <xul:iframe> which has context="inspector-node-popup" which defines the context menu.

As another example, context-clicking in the actual CSS rule view shows a context menu because the CssRuleView constructor does this:

  this.element.addEventListener("contextmenu", this._onContextMenu);

and has a bunch of manual logic in that event listener to do the context menu.

The good news is this all means you can customize your context menu exactly as you wish.

The bad news is it means you have to implement a context menu yourself...
Flags: needinfo?(bzbarsky)
Assignee

Comment 51

4 years ago
Attachment #8576541 - Attachment is obsolete: true
Attachment #8582893 - Attachment is obsolete: true
Attachment #8585778 - Flags: review?(bgrinstead)
Assignee

Comment 52

4 years ago
Attachment #8585781 - Flags: review?(bgrinstead)
Assignee

Updated

4 years ago
Attachment #8585782 - Flags: review?(bgrinstead)
Assignee

Comment 54

4 years ago
Attachment #8585783 - Flags: review?(bgrinstead)
Assignee

Comment 55

4 years ago
Rebased
Attachment #8586406 - Flags: review?(bgrinstead)
Assignee

Updated

4 years ago
Attachment #8585783 - Attachment is obsolete: true
Attachment #8585783 - Flags: review?(bgrinstead)
Assignee

Updated

4 years ago
Attachment #8585778 - Flags: review?(bgrinstead)
Assignee

Updated

4 years ago
Attachment #8585781 - Flags: review?(bgrinstead)
Assignee

Updated

4 years ago
Attachment #8585782 - Flags: review?(bgrinstead)
Assignee

Updated

4 years ago
Attachment #8586406 - Flags: review?(bgrinstead)
Assignee

Comment 56

4 years ago
Attachment #8585778 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8587670 - Flags: review?(bgrinstead)
Assignee

Comment 57

4 years ago
Attachment #8585781 - Attachment is obsolete: true
Attachment #8587672 - Flags: review?(bgrinstead)
Assignee

Comment 58

4 years ago
Attachment #8585782 - Attachment is obsolete: true
Attachment #8587673 - Flags: review?(bgrinstead)
Assignee

Comment 59

4 years ago
Attachment #8586406 - Attachment is obsolete: true
Attachment #8587674 - Flags: review?(bgrinstead)
We discussed a style issue where the computed view textbox is hitting the bottom of the toolbar.  It's because of the checkbox next to it (which is taking up 24px due to line-height on devtools-toolbar).  I guess we don't have any other toolbar that has a child using the full line height.

Oddly enough setting the line-height on the checkbox to '-moz-block-height' seems to fix the problem, but I'm not really sure what that is doing.
Assignee

Updated

4 years ago
Attachment #8587670 - Flags: review?(bgrinstead)
Assignee

Updated

4 years ago
Attachment #8587672 - Flags: review?(bgrinstead)
Assignee

Updated

4 years ago
Attachment #8587673 - Flags: review?(bgrinstead)
Assignee

Updated

4 years ago
Attachment #8587674 - Flags: review?(bgrinstead)
Assignee

Comment 61

4 years ago
Attachment #8587670 - Attachment is obsolete: true
Attachment #8587705 - Attachment is obsolete: true
Assignee

Comment 62

4 years ago
Attachment #8587672 - Attachment is obsolete: true
Assignee

Comment 64

4 years ago
Attachment #8587674 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8588917 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8588914 - Flags: review?(bgrinstead)
Assignee

Updated

4 years ago
Attachment #8588915 - Flags: review?(bgrinstead)
Assignee

Updated

4 years ago
Attachment #8588916 - Flags: review?(bgrinstead)
Assignee

Updated

4 years ago
Attachment #8588944 - Flags: review?(bgrinstead)
Whiteboard: [devedition-40]
Assignee

Updated

4 years ago
Attachment #8589078 - Flags: review?(bgrinstead)
Assignee

Comment 69

4 years ago
Was having some clipboard issues with try builds. Added copying from the context menu in the test cases.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed44efbebd37
Attachment #8589078 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Duplicate of this bug: 931386
Assignee

Updated

4 years ago
Attachment #8589298 - Flags: review?(bgrinstead)
Assigning P1 priority for some devedition-40 bugs. 

Filter on '148DD49E-D162-440B-AE28-E22632FC20AC'
Priority: -- → P1
Whiteboard: [devedition-40] → [devedition-40][difficulty=hard]
Comment on attachment 8588914 [details] [diff] [review]
Part 1: Implement filter styles in rule view v3

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

Very nice!  Only minor stuff, plus I'd like to clarify what some of the logic in highlightRules is doing.

::: browser/devtools/styleinspector/rule-view.js
@@ +2043,5 @@
> +    for (let textProp of aRule.textProps) {
> +      // Get the actual property value displayed in the rule view
> +      let propertyValue = textProp.editor.valueSpan.textContent;
> +
> +      // If the search value matches a property line, check if there is a exact

grammar nit: 'an'

@@ +2046,5 @@
> +
> +      // If the search value matches a property line, check if there is a exact
> +      // match for the property name and value in the rule property.
> +      let matchTextProperty = propertyMatch && name && value &&
> +           textProp.name.toLowerCase().contains(name) &&

I'd like to see a `let propertyName = textProp.name;` right after the propertyValue initialization, just for consistency

@@ +2049,5 @@
> +      let matchTextProperty = propertyMatch && name && value &&
> +           textProp.name.toLowerCase().contains(name) &&
> +           propertyValue.toLowerCase().contains(value);
> +
> +      // Check if there is a match for the parsed search value which only

The distinction between matchTextProperty / matchNameOrValue is hard to follow.  Is there some difference between what these are doing vs consolidating into this only:

let matches = (name && textProp.name.toLowerCase().contains(name)) ||
              (value && propertyValue.toLowerCase().contains(value));

I don't doubt if there is a difference, I'm just not sure what it is.

::: browser/devtools/styleinspector/ruleview.css
@@ +11,5 @@
> +}
> +
> +body {
> +  margin: 0;
> +  display : flex;

Nit: space between :

@@ +17,5 @@
> +  height: 100%;
> +}
> +
> +#ruleview-container {
> +  -moz-user-select: text;

Why does this need the -moz-user-select?

::: browser/themes/shared/devtools/toolbars.inc.css
@@ +341,5 @@
>    margin-top: 1px;
>    margin-bottom: 1px;
>    padding: 0;
>    -moz-padding-start: 22px;
> +  -moz-padding-end: 4px;

This change seems unrelated.  Is it intentional?

@@ +360,5 @@
> +.devtools-searchinput:-moz-locale-dir(rtl) {
> +  background-position: calc(100% - 8px) center;
> +}
> +
> +.devtools-searchinput > .textbox-input-box > .textbox-search-icons > .textbox-search-icon {

This was previously

.devtools-searchinput > .textbox-input-box > .textbox-search-icons {	
  display: none;	
}

What's the reason for the change?

@@ +364,5 @@
> +.devtools-searchinput > .textbox-input-box > .textbox-search-icons > .textbox-search-icon {
> +  visibility: hidden;
> +}
> +
> +.devtools-searchbox {

Please add a comment explaining that searchbox is actually a container element, an html div.
Attachment #8588914 - Flags: review?(bgrinstead) → feedback+
Assignee

Comment 73

4 years ago
Comment on attachment 8588914 [details] [diff] [review]
Part 1: Implement filter styles in rule view v3

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

::: browser/devtools/styleinspector/rule-view.js
@@ +2043,5 @@
> +    for (let textProp of aRule.textProps) {
> +      // Get the actual property value displayed in the rule view
> +      let propertyValue = textProp.editor.valueSpan.textContent;
> +
> +      // If the search value matches a property line, check if there is a exact

Fixed

@@ +2046,5 @@
> +
> +      // If the search value matches a property line, check if there is a exact
> +      // match for the property name and value in the rule property.
> +      let matchTextProperty = propertyMatch && name && value &&
> +           textProp.name.toLowerCase().contains(name) &&

Fixed. I also moved toLowerCase() to these variables.

@@ +2049,5 @@
> +      let matchTextProperty = propertyMatch && name && value &&
> +           textProp.name.toLowerCase().contains(name) &&
> +           propertyValue.toLowerCase().contains(value);
> +
> +      // Check if there is a match for the parsed search value which only

matchTextProperty would match exactly for a property line. An example would be: "font-family: arial". In such a case, the user would be filtering specifically for this property name and value.

On the other hand, the user could enter a name or value. The CSS_PROP_RE will split a property line (ie, "font-family: arial") into its name and value. If the search input is not in the format of a property line, it will try to match the given search input with the name or value of a rule property. However, there may be cases where the user is looking specifically for a name or value, and the search input is in the format of a property line, but missing either name or value such as "font:" or ":100%". The latter case was an extra feature since there might be some repeating terms that appear in both the name and value, but you might only care about one of them.

::: browser/devtools/styleinspector/ruleview.css
@@ +11,5 @@
> +}
> +
> +body {
> +  margin: 0;
> +  display : flex;

Fixed

@@ +17,5 @@
> +  height: 100%;
> +}
> +
> +#ruleview-container {
> +  -moz-user-select: text;

Fixed. Removed -moz-user-select since this is already applied to the container from .ruleview.

::: browser/themes/shared/devtools/toolbars.inc.css
@@ +341,5 @@
>    margin-top: 1px;
>    margin-bottom: 1px;
>    padding: 0;
>    -moz-padding-start: 22px;
> +  -moz-padding-end: 4px;

This is intentional change for search box you see in debugger, inspector, canvas debugger, etc. Since we unhide the search clear button, it is not probably aligned.

@@ +360,5 @@
> +.devtools-searchinput:-moz-locale-dir(rtl) {
> +  background-position: calc(100% - 8px) center;
> +}
> +
> +.devtools-searchinput > .textbox-input-box > .textbox-search-icons > .textbox-search-icon {

The original rule was removed to display the search clear button. This affects the searchbox you see in the debugger, inspector, etc.

In addition, there is an extra icon that appears in Windows that ntim mention in comment 19 that is hidden by the new rule.

@@ +364,5 @@
> +.devtools-searchinput > .textbox-input-box > .textbox-search-icons > .textbox-search-icon {
> +  visibility: hidden;
> +}
> +
> +.devtools-searchbox {

Fixed. Added comment.
Assignee

Comment 74

4 years ago
Attachment #8588914 - Attachment is obsolete: true
Attachment #8591201 - Flags: review?(bgrinstead)
Assignee

Comment 75

4 years ago
I modified highlightRules to exclude highlighting the inline element selector text by setting the selectorNodes to an empty array if the rule type is ELEMENT_STYLE. I figured highlighting 'element' would not be necessary since you will always see it, but you might still want to highlight properties within the inline style.
Attachment #8591201 - Attachment is obsolete: true
Attachment #8591201 - Flags: review?(bgrinstead)
Attachment #8591345 - Flags: review?(bgrinstead)
Assignee

Comment 76

4 years ago
Revised the logic for matchTextProperty / matchNameOrValue with https://gist.github.com/bgrins/b8fd30a74994e8360769. 

Follow up on the CSS_PROP_RE is to use the parseDeclarations:
let declarations = parseDeclarations(aValue);
let propertyMatch = declarations.length == 1 && declarations[0].name && declarations[0].value;
let name = propertyMatch ? declarations[0].name : aValue;
let value = propertyMatch ? declarations[0].value : aValue;

We decided not to use parseDeclaration for the time being because we don't get the desired name and value in order to handle cases seen in tests filter-7 and filter-8.
Attachment #8591345 - Attachment is obsolete: true
Attachment #8591345 - Flags: review?(bgrinstead)
Attachment #8591934 - Flags: review?(bgrinstead)
Comment on attachment 8588915 [details] [diff] [review]
Part 2: Add unit tests for filter styles in rule view v3

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

::: browser/devtools/styleinspector/test/browser_ruleview_search-filter_01.js
@@ +21,5 @@
> +add_task(function*() {
> +  yield addTab("data:text/html;charset=utf-8,test rule view search filter");
> +
> +  info("Creating the test document");
> +  content.document.body.innerHTML = PAGE_CONTENT;

This call ends up with 'unsafe CPOW usage' warnings when running in e10s mode.  It'd be better to include the content inside of the test uri, sort of like some of the inspector tests do: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/test/browser_inspector_reload-01.js#13

@@ +36,5 @@
> +function* testAddTextInFilter(inspector, ruleView) {
> +  info("Setting filter text to \"00F\"");
> +
> +  let searchField = ruleView.searchField;
> +  let onRuleViewFiltered = inspector.once("ruleview-filtered");;

Nit: double semicolon (repeated in all the tests)

@@ +40,5 @@
> +  let onRuleViewFiltered = inspector.once("ruleview-filtered");;
> +  searchField.focus();
> +
> +  let win = ruleView.doc.defaultView;
> +  EventUtils.synthesizeKey("0", {}, win);

Nit: throughout these tests, you could do something like this instead of calling synthesizeKey once per line:

  for (let key of "00F".split()) {
    EventUtils.synthesizeKey(key, {}, win);
  }

@@ +48,5 @@
> +  yield onRuleViewFiltered;
> +
> +  info("Check that the correct rules are visible");
> +  is(ruleView.element.children.length, 2, "Should have 2 rules.");
> +  is(ruleView.element.children[0]._ruleEditor.rule.selectorText, "element", "First rule is inline element.");

Throughout all these tests, calls to `ruleView.element.children[N]._ruleEditor` can be replaced with `getRuleViewRuleEditor(N)`

::: browser/devtools/styleinspector/test/browser_ruleview_search-filter_clear.js
@@ +52,5 @@
> +  is(ruleView.element.children.length, 2, "Should have 2 rules.");
> +  is(ruleView.element.children[0]._ruleEditor.rule.selectorText, "element", "First rule is inline element.");
> +  is(ruleView.element.children[1]._ruleEditor.rule.selectorText, "#testid", "Second rule is #testid.");
> +  ok(ruleView.element.children[1]._ruleEditor.rule.textProps[0].editor.element.classList.contains("ruleview-highlight"),
> +    "background-color text property is correctly highlighted.");

We should add a check to make sure that removing some of the filter will readd things that now match the new filter.  Specifically, we could press backspace so that the contents end up being '00' and make sure that the width: 100% is also visible.

@@ +70,5 @@
> +  yield onRuleViewFiltered;
> +
> +  info("Check the search filter is cleared and no rules are highlighted");
> +  ok(!searchField.value, "Search filter is cleared");
> +  ok(!doc.querySelectorAll(".ruleview-highlight").length &&

This should also make sure that the all of the rules are there in the rule view
Attachment #8588915 - Flags: review?(bgrinstead) → feedback+
Comment on attachment 8591934 [details] [diff] [review]
Part 1: Implement filter styles in rule view v6

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

Looks good!  r+ with just a nit

::: browser/devtools/styleinspector/rule-view.js
@@ +1603,5 @@
> +    }
> +
> +    let filterTimeout = (this.searchField.value.length > 0)
> +      ? FILTER_CHANGED_TIMEOUT : 0;
> +    this.searchClearButton.style.display = (this.searchField.value.length > 0)

Nit: believe this could be `this.searchClearButton.hidden = this.searchField.value.length === 0`
Attachment #8591934 - Flags: review?(bgrinstead) → review+
Comment on attachment 8588916 [details] [diff] [review]
Part 3: Adjust the styles in the computed view's filter style search v3

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

Nice!  Only minor things, feel free to reupload with r=me

::: browser/devtools/styleinspector/computed-view.js
@@ +157,5 @@
>  
> +  this.root = this.styleDocument.getElementById("root");
> +  this.element = this.styleDocument.getElementById("propertyContainer");
> +  this.searchField = this.styleDocument.getElementById("computedview-searchbox");
> +  this.searchClearButton = this.styleDocument.getElementById("computedview-searchinput-clear");

Nit: 80 characters.. could be mostly worked around by assigning `let doc = this.styleDocument`

@@ +524,5 @@
>      }
>  
> +    let filterTimeout = (this.searchField.value.length > 0)
> +      ? FILTER_CHANGED_TIMEOUT : 0;
> +    this.searchClearButton.style.display = (this.searchField.value.length > 0)

Nit: believe this could be `this.searchClearButton.hidden = this.searchField.value.length === 0`

::: browser/devtools/styleinspector/test/browser_computedview_search-filter_clear.js
@@ +9,5 @@
> +add_task(function*() {
> +  yield addTab("data:text/html;charset=utf-8,default styles test");
> +
> +  info("Creating the test document");
> +  content.document.body.innerHTML = '<style type="text/css"> ' +

Unsafe CPOW usage here, please set html by using the data url in addTab

@@ +13,5 @@
> +  content.document.body.innerHTML = '<style type="text/css"> ' +
> +    '.matches {color: #F00;}</style>' +
> +    '<span id="matches" class="matches">Some styled text</span>' +
> +    '</div>';
> +  content.document.title = "Style Inspector Search Filter Test";

Ditto

@@ +39,5 @@
> +
> +  searchField.focus();
> +
> +  let win = computedView.styleWindow;
> +  EventUtils.synthesizeKey("c", {}, win);

Could use:

  for (let key of "color".split()) {
    EventUtils.synthesizeKey(key, {}, win);
  }

@@ +50,5 @@
> +
> +  info("check that the correct properties are visible");
> +
> +  let propertyViews = computedView.propertyViews;
> +  propertyViews.forEach(function(propView) {

Nit: use arrow function for consistency

@@ +71,5 @@
> +
> +  yield onRefreshed;
> +
> +  info("check the search filter is cleared");
> +  ok(!searchField.value, "Search filter is cleared");

Can you also go through and make sure each propertyView is visible at this point?
Attachment #8588916 - Flags: review?(bgrinstead) → review+
Comment on attachment 8589298 [details] [diff] [review]
Part 4: Add textbox context menu for rule and computed view v3

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

This needs rebasing for toolbox.js, but looks good otherwise.

I'd like your thoughts about extracting the searchbox functionality into a shared widget (that handles clear button, context menu, timeout for search, etc).  At that point, we could have a single test in place for that widget that would replace both browser_computedview_search-filter_context-menu.js and browser_ruleview_search-filter_context-menu.js.  I don't want to block landing this bug on that work, but I think it's pretty clear based on how similar those tests are that we want identical behavior in these two places.

::: browser/devtools/framework/toolbox.js
@@ +329,5 @@
>  
>          let framesMenu = this.doc.getElementById("command-button-frames");
>          framesMenu.addEventListener("command", this.selectFrame, true);
>  
> +        this.textboxContextMenuPopup = this.doc.getElementById("toolbox-textbox-context-popup");

Nit: 80 chars

::: browser/devtools/styleinspector/computed-view.js
@@ +835,5 @@
> +   */
> +  _onFilterTextboxContextMenu: function(event) {
> +    try {
> +      this.styleDocument.defaultView.focus();
> +      this.searchContextMenu.openPopupAtScreen(event.screenX, event.screenY, true);

I don't think this.searchContextMenu is ever going to be needed elsewhere in this file..  Because of that I think it might be easier to follow if we just use `this.inspector.toolbox.textboxContextMenuPopup` directly here and don't bother with setting it in init / destroy

::: browser/devtools/styleinspector/rule-view.js
@@ +1631,5 @@
> +   */
> +  _onFilterTextboxContextMenu: function(event) {
> +    try {
> +      this.doc.defaultView.focus();
> +      this.searchContextMenu.openPopupAtScreen(event.screenX, event.screenY, true);

See comment about this.searchContextMenu in computed-view.js

::: browser/devtools/styleinspector/test/browser_computedview_search-filter_context-menu.js
@@ +18,5 @@
> +  yield selectNode("h1", inspector);
> +
> +  let win = view.styleWindow;
> +  let searchField = view.searchField;
> +  let searchContextMenu = view.searchContextMenu;

Can use toolbox.textboxContextMenuPopup here

@@ +19,5 @@
> +
> +  let win = view.styleWindow;
> +  let searchField = view.searchField;
> +  let searchContextMenu = view.searchContextMenu;
> +  ok(searchContextMenu, "The search filter context menu is loaded in the rule view");

rule view -> computed view

::: browser/devtools/styleinspector/test/browser_ruleview_search-filter_context-menu.js
@@ +18,5 @@
> +  yield selectNode("h1", inspector);
> +
> +  let win = view.doc.defaultView;
> +  let searchField = view.searchField;
> +  let searchContextMenu = view.searchContextMenu;

Can use toolbox.textboxContextMenuPopup here
Attachment #8589298 - Flags: review?(bgrinstead) → feedback+
Assignee

Comment 81

4 years ago
Attachment #8591934 - Attachment is obsolete: true
Attachment #8592102 - Flags: review+
Assignee

Comment 82

4 years ago
(In reply to Brian Grinstead [:bgrins] from comment #78)
> Comment on attachment 8591934 [details] [diff] [review]
> Part 1: Implement filter styles in rule view v6
> 
> Review of attachment 8591934 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good!  r+ with just a nit
> 
> ::: browser/devtools/styleinspector/rule-view.js
> @@ +1603,5 @@
> > +    }
> > +
> > +    let filterTimeout = (this.searchField.value.length > 0)
> > +      ? FILTER_CHANGED_TIMEOUT : 0;
> > +    this.searchClearButton.style.display = (this.searchField.value.length > 0)
> 
> Nit: believe this could be `this.searchClearButton.hidden =
> this.searchField.value.length === 0`

Fixed in v7. Diff: https://bugzilla.mozilla.org/attachment.cgi?oldid=8591934&action=interdiff&newid=8592102&headers=1
Assignee

Comment 83

4 years ago
Comment on attachment 8588915 [details] [diff] [review]
Part 2: Add unit tests for filter styles in rule view v3

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

::: browser/devtools/styleinspector/test/browser_ruleview_search-filter_01.js
@@ +21,5 @@
> +add_task(function*() {
> +  yield addTab("data:text/html;charset=utf-8,test rule view search filter");
> +
> +  info("Creating the test document");
> +  content.document.body.innerHTML = PAGE_CONTENT;

Fixed. Added all the content inside of the TEST_URI

@@ +36,5 @@
> +function* testAddTextInFilter(inspector, ruleView) {
> +  info("Setting filter text to \"00F\"");
> +
> +  let searchField = ruleView.searchField;
> +  let onRuleViewFiltered = inspector.once("ruleview-filtered");;

Fixed. Removed double semicolon

@@ +40,5 @@
> +  let onRuleViewFiltered = inspector.once("ruleview-filtered");;
> +  searchField.focus();
> +
> +  let win = ruleView.doc.defaultView;
> +  EventUtils.synthesizeKey("0", {}, win);

Fixed. Added synthesizeKeys in head.js

@@ +48,5 @@
> +  yield onRuleViewFiltered;
> +
> +  info("Check that the correct rules are visible");
> +  is(ruleView.element.children.length, 2, "Should have 2 rules.");
> +  is(ruleView.element.children[0]._ruleEditor.rule.selectorText, "element", "First rule is inline element.");

Fixed. Replaced with getRuleViewRuleEditor.

::: browser/devtools/styleinspector/test/browser_ruleview_search-filter_clear.js
@@ +52,5 @@
> +  is(ruleView.element.children.length, 2, "Should have 2 rules.");
> +  is(ruleView.element.children[0]._ruleEditor.rule.selectorText, "element", "First rule is inline element.");
> +  is(ruleView.element.children[1]._ruleEditor.rule.selectorText, "#testid", "Second rule is #testid.");
> +  ok(ruleView.element.children[1]._ruleEditor.rule.textProps[0].editor.element.classList.contains("ruleview-highlight"),
> +    "background-color text property is correctly highlighted.");

Fixed. Added a test for adding and editing the search filter value in filter-10

@@ +70,5 @@
> +  yield onRuleViewFiltered;
> +
> +  info("Check the search filter is cleared and no rules are highlighted");
> +  ok(!searchField.value, "Search filter is cleared");
> +  ok(!doc.querySelectorAll(".ruleview-highlight").length &&

Fixed. Added check for the number of rules visible after clearing
Assignee

Comment 84

4 years ago
Rebased Part 1
Attachment #8592102 - Attachment is obsolete: true
Attachment #8592648 - Flags: review+
Assignee

Comment 85

4 years ago
Attachment #8588915 - Attachment is obsolete: true
Attachment #8592649 - Flags: review?(bgrinstead)
Assignee

Comment 86

4 years ago
Comment on attachment 8588916 [details] [diff] [review]
Part 3: Adjust the styles in the computed view's filter style search v3

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

::: browser/devtools/styleinspector/computed-view.js
@@ +157,5 @@
>  
> +  this.root = this.styleDocument.getElementById("root");
> +  this.element = this.styleDocument.getElementById("propertyContainer");
> +  this.searchField = this.styleDocument.getElementById("computedview-searchbox");
> +  this.searchClearButton = this.styleDocument.getElementById("computedview-searchinput-clear");

Fixed. Used `let doc = this.styleDocument`

@@ +524,5 @@
>      }
>  
> +    let filterTimeout = (this.searchField.value.length > 0)
> +      ? FILTER_CHANGED_TIMEOUT : 0;
> +    this.searchClearButton.style.display = (this.searchField.value.length > 0)

Fixed. Used .hidden

::: browser/devtools/styleinspector/test/browser_computedview_search-filter_clear.js
@@ +9,5 @@
> +add_task(function*() {
> +  yield addTab("data:text/html;charset=utf-8,default styles test");
> +
> +  info("Creating the test document");
> +  content.document.body.innerHTML = '<style type="text/css"> ' +

Fixed. Added html in data uri for addTab

@@ +13,5 @@
> +  content.document.body.innerHTML = '<style type="text/css"> ' +
> +    '.matches {color: #F00;}</style>' +
> +    '<span id="matches" class="matches">Some styled text</span>' +
> +    '</div>';
> +  content.document.title = "Style Inspector Search Filter Test";

Fixed. Removed title

@@ +39,5 @@
> +
> +  searchField.focus();
> +
> +  let win = computedView.styleWindow;
> +  EventUtils.synthesizeKey("c", {}, win);

Fixed. Used new synthesizeKeys method defined in head.js

@@ +50,5 @@
> +
> +  info("check that the correct properties are visible");
> +
> +  let propertyViews = computedView.propertyViews;
> +  propertyViews.forEach(function(propView) {

Fixed. Used arrow function

@@ +71,5 @@
> +
> +  yield onRefreshed;
> +
> +  info("check the search filter is cleared");
> +  ok(!searchField.value, "Search filter is cleared");

Fixed. Added check to make sure each propertyView is visible.
Assignee

Updated

4 years ago
Attachment #8588916 - Attachment is obsolete: true
Assignee

Comment 88

4 years ago
Comment on attachment 8589298 [details] [diff] [review]
Part 4: Add textbox context menu for rule and computed view v3

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

::: browser/devtools/framework/toolbox.js
@@ +329,5 @@
>  
>          let framesMenu = this.doc.getElementById("command-button-frames");
>          framesMenu.addEventListener("command", this.selectFrame, true);
>  
> +        this.textboxContextMenuPopup = this.doc.getElementById("toolbox-textbox-context-popup");

Fixed.

::: browser/devtools/styleinspector/computed-view.js
@@ +835,5 @@
> +   */
> +  _onFilterTextboxContextMenu: function(event) {
> +    try {
> +      this.styleDocument.defaultView.focus();
> +      this.searchContextMenu.openPopupAtScreen(event.screenX, event.screenY, true);

Fixed. Removed this.searchContextMenu in computed view.

::: browser/devtools/styleinspector/rule-view.js
@@ +1631,5 @@
> +   */
> +  _onFilterTextboxContextMenu: function(event) {
> +    try {
> +      this.doc.defaultView.focus();
> +      this.searchContextMenu.openPopupAtScreen(event.screenX, event.screenY, true);

Fixed. Removed this.searchContextMenu in rule-view.js

::: browser/devtools/styleinspector/test/browser_computedview_search-filter_context-menu.js
@@ +18,5 @@
> +  yield selectNode("h1", inspector);
> +
> +  let win = view.styleWindow;
> +  let searchField = view.searchField;
> +  let searchContextMenu = view.searchContextMenu;

Fixed. Used toolbox.textboxContextMenuPopup

@@ +19,5 @@
> +
> +  let win = view.styleWindow;
> +  let searchField = view.searchField;
> +  let searchContextMenu = view.searchContextMenu;
> +  ok(searchContextMenu, "The search filter context menu is loaded in the rule view");

Fixed.

::: browser/devtools/styleinspector/test/browser_ruleview_search-filter_context-menu.js
@@ +18,5 @@
> +  yield selectNode("h1", inspector);
> +
> +  let win = view.doc.defaultView;
> +  let searchField = view.searchField;
> +  let searchContextMenu = view.searchContextMenu;

Fixed. Used toolbox.textboxContextMenuPopup
Assignee

Comment 89

4 years ago
Attachment #8589298 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8592715 - Flags: review?(bgrinstead)
Assignee

Comment 91

4 years ago
Comment on attachment 8592715 [details] [diff] [review]
Part 4: Add textbox context menu for rule and computed view v4

@shorlander, can you do a ui-review with the above build to ensure it meets expectations? I am a bit curious if the colours need to be adjusted with the new colour changes to the theme.
Attachment #8592715 - Flags: ui-review?(shorlander)
Comment on attachment 8592649 [details] [diff] [review]
Part 2: Add unit tests for filter styles in rule view v4

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

::: browser/devtools/styleinspector/test/browser_ruleview_search-filter_01.js
@@ +25,5 @@
> +  yield testAddTextInFilter(inspector, view);
> +});
> +
> +function* testAddTextInFilter(inspector, ruleView) {
> +  info("Setting filter text to \"00F\"");

Nit: I'm not a huge fan of having a duplicated string in this info() call in all of these tests.  In my experience, these can fall out of sync if / when the code changes.  Looking at when it's added in these tests, we could add an info() to the top of the synthesizeKeys function: `info ("About to synthesize keys: " + input.split(""));`

r+ either way, but that's my preference.
Attachment #8592649 - Flags: review?(bgrinstead) → review+
Comment on attachment 8592878 [details] [diff] [review]
Part 5: Refactor style inspector tests to use synthesizeKeys v1

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

Looks good
Attachment #8592878 - Flags: review?(bgrinstead) → review+
(In reply to Brian Grinstead [:bgrins] from comment #80)
> I'd like your thoughts about extracting the searchbox functionality into a
> shared widget (that handles clear button, context menu, timeout for search,
> etc).  At that point, we could have a single test in place for that widget
> that would replace both browser_computedview_search-filter_context-menu.js
> and browser_ruleview_search-filter_context-menu.js.  I don't want to block
> landing this bug on that work, but I think it's pretty clear based on how
> similar those tests are that we want identical behavior in these two places.

Filed 1154789 for discussion about this widget
Attachment #8592715 - Flags: review?(bgrinstead) → review+

Updated

4 years ago
Blocks: 1154809
Assignee

Updated

4 years ago
Blocks: 987365
Comment on attachment 8592715 [details] [diff] [review]
Part 4: Add textbox context menu for rule and computed view v4

Looks good to me. I did notice a problem with the clear icon being off-center but looks to be a deeper problem. Will file a follow-up.
Attachment #8592715 - Flags: ui-review?(shorlander) → ui-review+
Assignee

Updated

4 years ago
Keywords: checkin-needed
Assignee

Updated

4 years ago
Keywords: checkin-needed
Assignee

Comment 97

4 years ago
Rebased
Attachment #8592715 - Attachment is obsolete: true
Attachment #8593505 - Flags: review+
Assignee

Updated

4 years ago
Whiteboard: [devedition-40][difficulty=hard] → [devedition-40][difficulty=hard][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/f8326cd3e1ee
https://hg.mozilla.org/mozilla-central/rev/2ca9a3a1fc11
https://hg.mozilla.org/mozilla-central/rev/d78c245949df
https://hg.mozilla.org/mozilla-central/rev/0c1358f743a9
https://hg.mozilla.org/mozilla-central/rev/fc56554ea439
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [devedition-40][difficulty=hard][fixed-in-fx-team] → [devedition-40][difficulty=hard]
Target Milestone: --- → Firefox 40
See Also: → 1164343
Flags: qe-verify+
Assignee

Updated

4 years ago
No longer blocks: 1167669
Whiteboard: [devedition-40][difficulty=hard] → [polish-backlog][difficulty=hard]
Verified fixed on Firefox 40 RC, build ID: 20150807085045.
Also verified using latest 41.0a2 Aurora, for the DevEdition theme.
Status: RESOLVED → VERIFIED

Updated

3 years ago
Depends on: 1327692

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.