1210563 - Links at the top can overlap title

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[Will Bamberg [:wbamberg]]

Attachment: webide-bug.png

See attached screenshot.

Comment 1

[Manav Batra]

Hey, I'd like to try my hand at this bug. 
Could you point me to the file I shall be looking at for the code?

Comment 2

[Mike Hoye [:mhoye]]

Solid question, Manav. Jryans?

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Ah, sorry!  Seems the original comment slipped through my deluge of bug mail.

To get started with the code base, take a look at our Hacking page[1].  That page is Mercurial-focused, but also has good general advice.  If you prefer to use Git, you can also check the new Contributing page[2].

To reach the screen from comment 0, you would:

1. Open WebIDE (Tools -> Web Developer -> WebIDE)
2. Go to the menu bar
3. Project menu -> Preferences

However, there are many pages that share the same styling, see also Project menu -> Manage Extra Components and others.

You can find the HTML for the preferences page[3] at devtools/client/webide/content/prefs.xhtml.  The CSS you will probably need to modify[4] is at devtools/client/webide/themes/deck.css.

If you have more questions, be sure to set the needinfo? flag at the bottom to "mentor" so I don't miss them.  If you come up with patch, also make to set either feedback (you'd like some advice) or review (you believe it's done) flags on the patch to my email.

[1]: https://wiki.mozilla.org/DevTools/Hacking
[2]: https://developer.mozilla.org/en-US/docs/Tools/Contributing
[3]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/webide/content/prefs.xhtml
[4]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/webide/themes/deck.css

Comment 4

[Sunny Sidhu]

Hi, can I be assigned to this bug? I'm new and am having trouble finding a first bug to work on that's not already taken!

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Sunny Sidhu from comment #4)
> Hi, can I be assigned to this bug? I'm new and am having trouble finding a
> first bug to work on that's not already taken!

Our team usually waits for a first patch to be attached before assigning a bug.  Feel free to give it a try.

Comment 6

[Sunny Sidhu]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> Our team usually waits for a first patch to be attached before assigning a
> bug.  Feel free to give it a try.

I've solved the problem, but I'm struggling to find the 'right' way to create a patch, I've read/seen 3 different methods. Could you point towards the right direction please?

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Sunny Sidhu from comment #6)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> > Our team usually waits for a first patch to be attached before assigning a
> > bug.  Feel free to give it a try.
> 
> I've solved the problem, but I'm struggling to find the 'right' way to
> create a patch, I've read/seen 3 different methods. Could you point towards
> the right direction please?

Hmm, there is a lot of different documentation, especially because there are both Git-based and Mercurial-based workflows.  And even within those, different approaches...

Anyway, I linked to both of those above in comment 3.  I'm not sure I know of other guides than those.

If you're not sure how to proceed, I'd recommend joining Mozilla IRC[1] on the #introduction channel where you can get some help in real time.

Or if that fails, try to describe to me what you've attempted, and we can try to work it out.

[1]: https://wiki.mozilla.org/Irc

Comment 8

[Sunny Sidhu]

Attachment: bug1210563fix.patch

Thanks, I was able to figure it out. Please find attached my patch.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8673286 [details] [diff] [review]
bug1210563fix.patch

Changing to feedback request.  I'll take a look soon!

Comment 10

[Sunny Sidhu]

Attachment: rev1 - Alters code for positioning

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8673291 [details] [diff] [review]
rev1 - Alters code for positioning

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

Great, that takes care of the main issue!  See comments below about a few details.

Also, please add "r=jryans" to the end of the commit message for the next version.

::: devtools/client/webide/themes/deck.css
@@ +35,5 @@
>    margin-bottom: .5em;
>  }
>  
>  #controls {
> +  float: right;

Overall, that works nicely!

To keep the same position of the elements, we could also add "position: relative", and change top and right here to -10px, since the body has 20px padding.

Or, we could live with 10px extra space on the top and right around the controls.  What do you think is better?

Comment 12

[Sunny Sidhu]

Attachment: rev2 - Amends position property to relative for controls element

I think the method you mentioned would be better. Float works by removing the element from the normal document flow and then adjusts how other elements drift around it. Whereas the position is more 'explicit' and nicer, because you can move the element relative to where it should normally appear. The top and right properties also are positional properties, so they don't actually apply to the float method. It's probably also better to keep the controls positioned where they were initially intended to be.

Please find attached my revised patch.

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8675303 [details] [diff] [review]
rev2 - Amends position property to relative for controls element

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

I should have mentioned this earlier on... I'm not sure how you've been testing changes so far, but there are two techniques available:

A. You can use the Browser Toolbox[1] to access other windows[2] like WebIDE
B. You can load WebIDE in a regular tab for inspection by going to chrome://webide/content/webide.xul in a tab
  * Some features of WebIDE may not work in this mode, but it's usually good enough for trying out CSS
  * You'll probably also need to change documents[2] here as well, since there are inner frames involved

[1]: https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox
[2]: https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox#Targeting_a_document

::: devtools/client/webide/themes/deck.css
@@ +35,5 @@
>    margin-bottom: .5em;
>  }
>  
>  #controls {
> +  position: relative;

That's almost what I had in mind, but I meant these 3 lines *plus* the float: right you already had.  At the moment, the controls aren't right-justified anymore without the float: right.

Sorry for not being clear!

If you add that line back in as well, it should position the controls right were they were, but also they'll push down the header at small widths, like we're hoping for here.

Comment 14

[Sunny Sidhu]

Attachment: rev 3 - Adds float property and amends position and margins

Thanks for the information on testing changes! The Browser Toolbox makes life so much more easier.

Please find attached my revised my patch.

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8681858 [details] [diff] [review]
rev 3 - Adds float property and amends position and margins

Great, this looks good to me!  Thanks for working on it.

I don't think there are any tests for this area, so I think we can land it directly.  I'll do that now.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/9e1f2504b081

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9e1f2504b081

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/9e1f2504b081

Comment 19

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion