Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Crop long CSS values #34

Closed
wants to merge 6 commits into from

4 participants

@farshidbeheshti

Issue 5862

@farshidbeheshti

Simon, it was a mistake by me. I've returned cancelEditing( ) function back

@SebastianZ
Owner

Committed the changes plus a little fix in cbc142e81d.

Sebastian

@SebastianZ SebastianZ closed this
@farshidbeheshti
@SebastianZ
Owner

In my commit I already replaced cssDisplayedPropertyValueLimit by stringCropLength. Sorry that I took over the last changes you were also working on.

Reusing extensions.firebug.stringCropLength for other parts of the UI is not part of issue 5862, so that should be done separately.
Furthermore I disagree with Honza that it should be used everywhere strings are cropped. E.g. source links need to have a shorter limit than URLs inside the Net panel.

Also note that I suggested enhanced string cropping to Mozilla some time ago and also to the W3C. Unfortunately nobody responded so far.

Sebastian

@janodvarko
Owner

source links need to have a shorter limit than URLs inside the Net panel.
Sure, we can have a different/pref category for strings that needs to be significantly shorther (or longer) and we should again try to share everywhere where possible.

Honza

@simonlindholm

I generally don't see why we want to share prefs for similarly-shortened strings. Say the CSS cropping begins to bug me, so I set the cropping pref to 0. But then there is the trade-off that lots of other string also don't crop!
There's no problem with more prefs, just increased customizability.

@janodvarko
Owner

Of course there are two approaches

1) new preference for every (different type of) cropped string in the UI so, the user can customize every bit of it.
2) Having 2-3 categories (e.g. small, normal, large) shared. In this case the user needs to change just these prefs to adjust string cropping and not zilliens of props.

I personally prefer #2

Honza

@SebastianZ
Owner

I generally don't see why we want to share prefs for similarly-shortened strings.

Generally we want to avoid confusion by adding too many preferences, especially for similar things. General prefs should be fine in this case. And if there's really a need for cropping specific strings individually, we can still add a property for them.

Sebastian

@simonlindholm

I personally prefer #2

If someone wants to customize cropping enough to go to about:config, they probably have a specific problem in mind (assuming it's set to '0' or a high value, at least), and are probably okay with wading through a few prefs.
Were the options visible through the UI I'd agree more.

Generally we want to avoid confusion by adding too many preferences, especially for similar things.

Prefs for similar things is fine, but that's not what extensions.firebug.stringCropLength is.

In any case, I think mainly that any particular cropping which could be annoying should be possible to turn off, and due to backgrounds with long, interesting values, and infotips, I think the CSS panel could be such a thing (though I've admittedly not used it since this change).

@simonlindholm

(also, I'll be away for the weekend, so I can't respond, sorry)

@SebastianZ
Owner

Created Issue 5898 for this. So we can continue the discussion there.

Sebastian

@SebastianZ
Owner

In any case, I think mainly that any particular cropping which could be annoying should be possible to turn off, and due to backgrounds with long, interesting values, and infotips, I think the CSS panel could be such a thing

I created issue 5880 recently, which describes another approach for this specific case.

Sebastian

@simonlindholm

I read through the code now, and I saw some issues with this commit:

  • getInitialValue() returns "" for disabled properties, so the pre-filled value of an edited property is "", and Esc doesn't revert it correctly. See e.g. the failing "Editing a disabled property makes another" test: https://getfirebug.com/testresults/?headerid=7e2a8dfa3cbde00b138c42d9fb3a4077 (the same bug is actually also in showInfoTip, so if you want to make a general function... :) )
  • disablePropertyRow() parses .textContent as the actual value, so enabling/disabling of cropped properties doesn't work.
  • The "change property name" also parses .textContent as an actual value (Farshid: you should have searched for such occurrences!), and fails for long property values.
  • Same thing with rule renaming.

Of course the real issue is that we pull raw information from the UI in the first place, but fixing that requires quite a bit of refactoring.

@SebastianZ
Owner

and I saw some issues with this commit:

Why didn't I see all this before? :-/

Of course the real issue problem is that we pull raw information from the UI in the first place, but fixing that
requires quite a bit of refactoring.

We have that at many places throughout Firebug and we need to change that step by step.
The big problem here is that disabled properties are not available through the repObject (the style rule). So we need to find a way to get that info.

Sebastian

@simonlindholm

Also units aren't stripped and it removes !important from edited properties... We should have backed this out for release.

@simonlindholm

And further, changing a property's name and pressing tab is regressed: when the old property value becomes invalid for the new property name the property is temporarily removed, so the initial value is "" instead of what is shown.

And the use of rangeOffset as an offset into the property value in showInfoTip obviously doesn't work with cropping.

... Let's just back this out and rethink our model here.

@farshidbeheshti

Continuation is here:
Pull request 41

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
63 extension/content/firebug/css/cssPanel.js
@@ -50,6 +50,14 @@ var CSSDomplateBase =
isSelectorEditable: function(rule)
{
return rule.isSelectorEditable && this.isEditable(rule);
+ },
+
+ getPropertyValue: function(prop)
+ {
+ var limit = Options.get("cssDisplayedPropertyValueLimit");
+ if (limit > 0)
+ return Str.cropString(prop.value, limit);
+ return prop.value;
}
};
@@ -70,7 +78,7 @@ var CSSPropTag = domplate(CSSDomplateBase,
// Use a space here, so that "copy to clipboard" has it (issue 3266).
SPAN({"class": "cssColon"}, ": "),
SPAN({"class": "cssPropValue", $editable: "$rule|isEditable"},
- "$prop.value$prop.important"
+ "$prop|getPropertyValue$prop.important"
),
SPAN({"class": "cssSemi"}, ";"
)
@@ -1286,10 +1294,11 @@ Firebug.CSSStyleSheetPanel.prototype = Obj.extend(Firebug.Panel,
var propValue = Dom.getAncestorByClass(target, "cssPropValue");
if (propValue)
{
- var text = propValue.textContent;
+ var styleRule = Firebug.getRepObject(target);
var prop = Dom.getAncestorByClass(target, "cssProp");
var propNameNode = prop.getElementsByClassName("cssPropName").item(0);
var propName = propNameNode.textContent.toLowerCase();
+ var text = styleRule.style.getPropertyValue(propName);
var cssValue;
if (propName == "font" || propName == "font-family")
@@ -1672,39 +1681,38 @@ CSSEditor.prototype = domplate(Firebug.InlineEditor.prototype,
this.panel.removeDisabledProperty(rule, propName);
}
- target.textContent = value;
-
if (rule instanceof window.CSSStyleRule || rule instanceof window.Element)
{
if (Css.hasClass(target, "cssPropName"))
{
-
+ target.textContent = value;
+
if (value && previousValue != value) // name of property has changed.
{
// Record the original CSS text for the inline case so we can reconstruct at a later
// point for diffing purposes
var baseText = rule.style ? rule.style.cssText : rule.cssText;
-
+
propValue = Dom.getChildByClass(row, "cssPropValue").textContent;
parsedValue = parsePriority(propValue);
-
+
if (FBTrace.DBG_CSS)
FBTrace.sysout("CSSEditor.saveEdit : " + previousValue + "->" + value +
" = " + propValue);
-
+
if (propValue && propValue != "undefined")
{
if (FBTrace.DBG_CSS)
FBTrace.sysout("CSSEditor.saveEdit : " + previousValue + "->" + value +
" = " + propValue);
-
+
if (previousValue)
CSSModule.removeProperty(rule, previousValue);
-
+
CSSModule.setProperty(rule, value, parsedValue.value,
parsedValue.priority);
}
-
+
Events.dispatch(CSSModule.fbListeners, "onCSSPropertyNameChanged", [rule, value,
previousValue, baseText]);
}
@@ -1716,16 +1724,19 @@ CSSEditor.prototype = domplate(Firebug.InlineEditor.prototype,
}
else if (Dom.getAncestorByClass(target, "cssPropValue"))
{
+ var limit = Options.get("cssDisplayedPropertyValueLimit");
+ target.textContent = limit > 0 ? Str.cropString(value, limit) : value;
+
propName = Dom.getChildByClass(row, "cssPropName").textContent;
propValue = Dom.getChildByClass(row, "cssPropValue").textContent;
-
+
if (FBTrace.DBG_CSS)
{
FBTrace.sysout("CSSEditor.saveEdit propName=propValue: "+propName +
" = "+propValue+"\n");
// FBTrace.sysout("CSSEditor.saveEdit BEFORE style:",style);
}
-
+
if (value && value != "null")
{
parsedValue = parsePriority(value);
@@ -1737,7 +1748,7 @@ CSSEditor.prototype = domplate(Firebug.InlineEditor.prototype,
CSSModule.removeProperty(rule, propName);
}
}
-
+
if (value)
{
var saveSuccess = !!rule.style.getPropertyValue(propName || value);
@@ -1747,11 +1758,11 @@ CSSEditor.prototype = domplate(Firebug.InlineEditor.prototype,
{
return match[1].toUpperCase()
});
-
+
if (propName in rule.style || propName == "float")
saveSuccess = "almost";
}
-
+
this.box.setAttribute("saveSuccess", saveSuccess);
}
else
@@ -1761,6 +1772,8 @@ CSSEditor.prototype = domplate(Firebug.InlineEditor.prototype,
}
else if (rule instanceof window.CSSImportRule && Css.hasClass(target, "cssMediaQuery"))
{
+ target.textContent = value;
+
if (FBTrace.DBG_CSS)
{
FBTrace.sysout("CSSEditor.saveEdit: @import media query: " +
@@ -1779,6 +1792,8 @@ CSSEditor.prototype = domplate(Firebug.InlineEditor.prototype,
}
else if (rule instanceof window.CSSCharsetRule)
{
+ target.textContent = value;
+
if (FBTrace.DBG_CSS)
FBTrace.sysout("CSSEditor.saveEdit: @charset: " + previousValue + "->" + value);
@@ -1826,6 +1841,22 @@ CSSEditor.prototype = domplate(Firebug.InlineEditor.prototype,
}
},
+ getInitialValue: function(target, value)
+ {
+ if (value == "")
+ return value;
+ var propValue = Dom.getAncestorByClass(target, "cssPropValue");
+ if (propValue)
+ {
+ var styleRule = Firebug.getRepObject(target);
+ var prop = Dom.getAncestorByClass(target, "cssProp");
+ var propNameNode = prop.getElementsByClassName("cssPropName").item(0);
+ var propName = propNameNode.textContent.toLowerCase();
+ value = styleRule.style.getPropertyValue(propName);
+ }
+ return value;
+ },
+
// * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
getAutoCompleteRange: function(value, offset)
View
1  extension/content/firebug/lib/options.js
@@ -52,6 +52,7 @@ const prefNames = // XXXjjb TODO distribute to modules
"expandShorthandProps",
"cssEditMode",
"colorDisplay",
+ "cssDisplayedPropertyValueLimit",
// Computed
"computedStylesDisplay",
View
1  extension/defaults/preferences/firebug.js
@@ -79,6 +79,7 @@ pref("extensions.firebug.showUserAgentCSS", false);
pref("extensions.firebug.expandShorthandProps", false);
pref("extensions.firebug.cssEditMode", "Source");
pref("extensions.firebug.colorDisplay", "hex");
+pref("extensions.firebug.cssDisplayedPropertyValueLimit", 100);
// Computed
pref("extensions.firebug.computedStylesDisplay", "grouped");
Something went wrong with that request. Please try again.