Closed Bug 788458 Opened 12 years ago Closed 12 years ago

Fix issues with copy / paste in rule and computed views

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 19

People

(Reporter: miker, Assigned: miker)

References

Details

Attachments

(1 file, 2 obsolete files)

We have various reoccurring issues with the current copy / paste functionality because it is dependent on the HTML structure of the tools themselves, which can be slightly different across operating systems.

This is extremely fragile code and a breeding ground for bugs.

We will remove the context menu and only allow basic copy / paste functionality. This simplified version should make these issues manageable.
Attached patch Reimplemented (obsolete) — Splinter Review
Originally due to the way focus worked in the rule view it was not possible to use the native copy / paste so we had to implement our own. Now the native copy / paste works fine so a lot of cruft could be removed and our tests are no longer needed.
Attachment #658460 - Flags: review?(paul)
That's a lot of code being removed :)

Can you describe exactly the features we will be missing?

Also, can we have some tests to valid that ctrl-c/ctrl-v work as expected?

What about the references (foo.css:42) and the rule titles (inherited from XXX)? The aren't removed from the clipboard, are they? An easy way to remove them is to make them `-moz-user-select: -moz-none`.
(In reply to Paul Rouget [:paul] from comment #2)
> That's a lot of code being removed :)
> 
> Can you describe exactly the features we will be missing?
> 

We will no longer have a context menu in the rule or computed views so no copy property, copy rule, copy selection etc.

> Also, can we have some tests to valid that ctrl-c/ctrl-v work as expected?
> 

We can but then we will have issues with different OSes again. I didn't include tests purely because copy / paste has a bunch of browser tests already and we are using the native copy / paste here.

> What about the references (foo.css:42) and the rule titles (inherited from
> XXX)? The aren't removed from the clipboard, are they? An easy way to remove
> them is to make them `-moz-user-select: -moz-none`.

We could do that ... of course, that will make it impossible for users to copy these references.
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #3)
> > What about the references (foo.css:42) and the rule titles (inherited from
> > XXX)? The aren't removed from the clipboard, are they? An easy way to remove
> > them is to make them `-moz-user-select: -moz-none`.
> 
> We could do that ... of course, that will make it impossible for users to
> copy these references.

You're right. But for sure, having these piece of text included will cause problems. Let skip them for now and see if someone raise a bug.

For the tests, I understand that. But we have a bad story with this feature. I'd feel much more comfortable with a simple ctrl-c/ctrl-v test.

About the missing features, make sure to file followup bugs.

Also, I'd prefer if you ask Dave for the review (I want him to validate this approach).

And can we ship Firefox 18 with no context menu?
(In reply to Paul Rouget [:paul] from comment #4)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #3)
> > > What about the references (foo.css:42) and the rule titles (inherited from
> > > XXX)? The aren't removed from the clipboard, are they? An easy way to remove
> > > them is to make them `-moz-user-select: -moz-none`.
> > 
> > We could do that ... of course, that will make it impossible for users to
> > copy these references.
> 
> You're right. But for sure, having these piece of text included will cause
> problems. Let skip them for now and see if someone raise a bug.
> 

As it happens these elements do have -moz-user-select: -moz-none set. It prevents you from selecting from the node directly but you can still drag from another node and it is still selected.

> For the tests, I understand that. But we have a bad story with this feature.
> I'd feel much more comfortable with a simple ctrl-c/ctrl-v test.
> 

I will look into creating a cross OS test.

> About the missing features, make sure to file followup bugs.
> 

Now alarm bells are starting to ring. The previous implementation was simple and worked fine, just not on all OSes. It was fragile but any copy / paste implementation would be. I am not sure that removing these features and then adding them again makes sense.

> Also, I'd prefer if you ask Dave for the review (I want him to validate this
> approach).
> 

Sure, I will CC him ... we can discuss it later.

> And can we ship Firefox 18 with no context menu?

I agree, I think we should keep the old context menu.
I am repurposing this bug as it makes no sense to tear out our current copy / paste functionality and reimplement it ... as it turns out, there are very few ways of implementing this and they all appear to be fragile.

We are better to fix our current bugs and increase test coverage. I have a new patch that I will add when it has finished running through try.
Summary: Re-implement copy / paste in rule and computed views → Fix issues with copy / paste in rule and computed views
Attachment #658460 - Attachment is obsolete: true
Attachment #658460 - Flags: review?(paul)
Dave, Paul said that he would prefer you to review this as you are more familiar with the code.

It was possible to click on a value to show an editor and then select all text in the rule. This meant that the text in the editor and the text in the rule view were both selected.

So this patch does 4 things:
1. Mousedown outside of an editor now closes the editor
2. We had a test that depended on an editor being open and text outside of the editor being selected. This is no longer possible so the test has been removed.
3. _onCopy() has been rewritten to allow copying within editors and generally made to work better across OSes.
4. An _onCopy() test has been added.

Works fine on my test boxes and green on try.
Attachment #660852 - Flags: review?(dcamp)
Blocks: 794254
Attachment #660852 - Flags: review?(dcamp) → review?(jwalker)
Attachment #660852 - Flags: review?(jwalker) → review?(dcamp)
Comment on attachment 660852 [details] [diff] [review]
Fixed bugs relating to copy / paste with editor involvement

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

::: browser/devtools/styleinspector/CssRuleView.jsm
@@ +1404,5 @@
> +                                                 ".ruleview-propertyvalue");
> +        for (let node of nodes) {
> +          if (node.inplaceEditor) {
> +            node.inplaceEditor._clear();
> +          }

Maybe the inplace editor should just put a copy of itself on the editor element...
Attachment #660852 - Flags: review?(dcamp) → review-
Seems to leave copy paste working well across platforms.

Also fixed copy property.
Attachment #660852 - Attachment is obsolete: true
Attachment #666583 - Flags: review?(dcamp)
Attachment #666583 - Flags: review?(dcamp) → review+
Whiteboard: [land-in-fx-team]
Backed out on suspicion of causing mochitest-1 and 3 permaorange:
https://hg.mozilla.org/integration/fx-team/rev/ac18200daa5e
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/b472143f21e1
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 19
Thanks, this is great!
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: