Closed Bug 1167601 Opened 9 years ago Closed 9 years ago

Convert newTab.xul to newTab.xhtml

Categories

(Firefox :: New Tab Page, defect)

Product:
Firefox
Component:
New Tab Page
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 42

People

(Reporter: mconley, Assigned: ursula)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat)

Attachments

(2 files, 7 obsolete files)

Convert newTab.xul to newTab.xhtml v2
21.70 KB, patch
Details | Diff | Splinter Review
MozReview Request: Bug 1167601 - Convert newTab.xul to newTab.xhtml v2
40 bytes, text/x-review-board-request
mconley
: review+
Details 
newTab.xul cannot be rendered properly in the content process. We should be using HTML there.

This will involve moving the panels out of content, and relying on Enn's work on bug 873923.
My bet is that the panels should be moved into the mainPopupSet in browser/base/content/browser.xul. We'll need to hook up some messaging between content and chrome in order to control those popups from content (content sends a message that a button was clicked, perhaps some other information to indicate what the panel should be showing, includes coordinates and dimensions for the panel to be opened, and then the parent does the actual job of opening the panel with openPopupAtScreenRect).
Depends on: 1167643

    
  

        Attachment #8610573 -
        Flags: review?(mconley)

    
    

    
  


  

    
  

        Attachment #8610573 -
        Attachment is obsolete: true

        Attachment #8610573 -
        Flags: review?(mconley)

    
    

    
  


  

    
  

        Attachment #8610771 -
        Attachment is obsolete: true

    
  

        Attachment #8610773 -
        Flags: review?(mconley)

    
    

    
  
Comment on attachment 8610773 [details] [diff] [review]
Convert newTab.xul to newTab.xhtml

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

Hey Ursula,

I think your technique is sound here - you should feel free to file and start working on the New Tab customization page.

We might need to rethink how this patch relates to the patch in bug 1167643 - like, I suspect that patch cannot land without this one, and we might want to break that dependency (so that bug 1167643 can land, and then converting newTab.xul to newTab.xhtml can occur independently).

For breaking up patches in hg, I recommend examining the "hg uncommit"[1] command that comes with the evolve extension, and hg crecord[2], which allows you to selectively commit various parts of a change.

I did a pretty high-level review here (though I couldn't resist tagging some style stuff too). I'll do a second pass soon-ish. In the meantime, feel free to update this patch with additional developments.

[1]: https://mercurial.selenic.com/wiki/EvolveExtension
[2]: https://mercurial.selenic.com/wiki/CrecordExtension

::: browser/base/content/browser.xul
@@ +253,5 @@
>          <label>&changeSearchSettings.button;</label>
>        </hbox>
>      </panel>
>  
> +    <panel id="newtab-search-panel" orient="vertical" type="arrow" hidden="true" onclick="this.hidePopup();">

Actually seems kind of strange to have this panel do pretty much exactly the same thing as the panel above it.

It might be worth while trying to combine these...

::: browser/base/content/newtab/newTab.xhtml
@@ +15,5 @@
>  
> +<html xmlns="http://www.w3.org/1999/xhtml">
> +    <head>
> +        <title>&newtab.pageTitle;</title>
> +        <!-- style sheets -->

Nit - we can probably do without this comment - I don't think it adds much value, to be honest.

@@ +24,5 @@
> +        <link rel="stylesheet" type="text/css" media="all"
> +              href="chrome://browser/content/newtab/newTab.css" />
> +        <link rel="stylesheet" type="text/css" media="all"
> +              href="chrome://browser/skin/newtab/newTab.css" />
> +    </head>            

Trailing whitespace.

@@ +25,5 @@
> +              href="chrome://browser/content/newtab/newTab.css" />
> +        <link rel="stylesheet" type="text/css" media="all"
> +              href="chrome://browser/skin/newtab/newTab.css" />
> +    </head>            
> +<!-- <xul:panel id="newtab-customize-panel" orient="vertical" type="arrow"

Commented out code will need to be removed before landing.

@@ +47,5 @@
>        <xul:label>&newtab.customize.cog.learn;</xul:label>
>      </xul:hbox>
> +  </xul:panel> -->
> +
> +  <body>

We'll need to update the indentation on the tags below body because of their new parent. I believe if you highlight the lines in Sublime, and hit >, it'll do it for you?

::: browser/base/content/newtab/search.js
@@ +39,5 @@
> +      y: rect.y,
> +      width: rect.width,
> +      height: rect.height
> +    });
> +    

Trailing whitespace

@@ +40,5 @@
> +      width: rect.width,
> +      height: rect.height
> +    });
> +    
> +    addMessageListener("NewTab:CloseSearchPanel", function(message){

We'll probably want to add this just once (perhaps in the initializer), as opposed to every time the panel is opened.

Also, please add a space before the {

::: browser/base/jar.mn
@@ +106,5 @@
>          content/browser/defaultthemes/5.icon.jpg      (content/defaultthemes/5.icon.jpg)
>          content/browser/defaultthemes/5.preview.jpg   (content/defaultthemes/5.preview.jpg)
>          content/browser/defaultthemes/devedition.header.png   (content/defaultthemes/devedition.header.png)
>          content/browser/defaultthemes/devedition.icon.png     (content/defaultthemes/devedition.icon.png)
> +        content/browser/newtab/newTab.xhtml             (content/newtab/newTab.xhtml)

Let's make the alignment uniform for the (content/newtab/newTab.xhtml) bits, if we can.

::: browser/modules/AboutNewTab.jsm
@@ +1,1 @@
> +"use strict";

Needs MPL license at the top - see AboutHome.jsm for example.

@@ +7,5 @@
> +this.EXPORTED_SYMBOLS = [ "AboutNewTab" ];
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/RemotePageManager.jsm");

If we can, we should use defineLazyModuleGetter for RemotePageManager.jsm as well, since we don't need it until init is called.

@@ +9,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/RemotePageManager.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "AppConstants", 

Trailing whitespace on 13, busted alignment on 14

@@ +18,5 @@
> +  "resource://gre/modules/FxAccounts.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Promise",
> +  "resource://gre/modules/Promise.jsm");
> +
> +var pageListener = null;

Might as well just make this a member of the AboutNewTab.

@@ +20,5 @@
> +  "resource://gre/modules/Promise.jsm");
> +
> +var pageListener = null;
> +
> +let AboutNewTab = {

Let's use two-space indentation here.

@@ +30,5 @@
> +
> +	openSearchPanel: function(message){
> +		let doc = message.target.browser.ownerDocument;
> +		let panel = doc.getElementById("newtab-search-panel");
> +		let arrow = doc.getAnonymousElementByAttribute(panel, "anonid", "arrow");

Why is the arrow being manipulated here? It only looks like we ever set hidden to false...

@@ +39,5 @@
> +		let height = message.data.height;
> +		let boxWidth = box.screenX;
> +		let boxHeight = box.screenY;
> +		panel.hidden = false;
> +		panel.openPopupAtScreenRect("panel", x + boxWidth + width * 0.5, y + boxHeight + height, width, height);

Can you add some documentation explaining the calculation?

@@ +40,5 @@
> +		let boxWidth = box.screenX;
> +		let boxHeight = box.screenY;
> +		panel.hidden = false;
> +		panel.openPopupAtScreenRect("panel", x + boxWidth + width * 0.5, y + boxHeight + height, width, height);
> +    	panel.addEventListener("popuphidden", function onHidden() {

Busted alignment lines 44-49.

        Attachment #8610773 -
        Flags: review?(mconley) → feedback+

    
    

    
  


  

    
  

        Attachment #8610773 -
        Attachment is obsolete: true

    
  

        Attachment #8616114 -
        Flags: review?(mconley)

    
    

    
  
Wait, why xhtml at all? Why not HTML?

    
    

    
  
I'm also seeing a lot of div's. Can't we use some structural elements?

    
    

    
  
(In reply to Marcos Caceres [:marcosc] from comment #7)
> Wait, why xhtml at all? Why not HTML?

I believe XHTML is being used to make use of the DTD's and locale stuff.

(In reply to Marcos Caceres [:marcosc] from comment #8)
> I'm also seeing a lot of div's. Can't we use some structural elements?

I'm all for it, but maybe as a follow-up - I'd rather not block this on that.

    
    

    
  
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #9)
> (In reply to Marcos Caceres [:marcosc] from comment #7)
> > Wait, why xhtml at all? Why not HTML?
> 
> I believe XHTML is being used to make use of the DTD's and locale stuff.

DTD's are pretty obsolete (as it's unlikely they are used as intended, if at all), so we should really stop using that. I need to check locale stuff tho.
 
> (In reply to Marcos Caceres [:marcosc] from comment #8)
> > I'm also seeing a lot of div's. Can't we use some structural elements?
> 
> I'm all for it, but maybe as a follow-up - I'd rather not block this on that.

Sure - I guess as long as the page remains accessible it doesn't make much of a difference.

    
    

    
  
Comment on attachment 8616114 [details] [diff] [review]
Convert newTab.xul to newTab.xhtml

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

Looking good! I assume the RTL stuff and flex stuff will be folded in on top?

::: browser/base/content/newtab/newTab.xhtml
@@ +3,5 @@
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
> +   <!DOCTYPE html [

Busted indentation - lines 7-14

@@ +59,5 @@
>        </div>
> +    </div>
> +
> +    <div id="newtab-scrollbox">
> +

Let's get rid of the newlines that were introduced here.

        Attachment #8616114 -
        Flags: review?(mconley) → feedback+

    
    

    
  


  

    
  

        Attachment #8616114 -
        Attachment is obsolete: true

    
  

        Attachment #8620951 -
        Flags: review?(mconley)

    
    

    
  
Comment on attachment 8620951 [details] [diff] [review]
Convert newTab.xul to newTab.xhtml

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

Looking good. Glad to see that the tile dragging / dropping is working. :)

I couldn't get RTL to work properly though... the customize button stayed on the right, the search button stayed on the right of the text input, the text input itself was LTR still. RTL is where I'd focus next here.

::: browser/base/content/newtab/newTab.css
@@ +8,5 @@
> +
> +body {
> +  width: 100%;
> +  height: 100%;
> +  padding: 0;

I suspect we'll want to get rid of the default margin on the body as well.

@@ +9,5 @@
> +body {
> +  width: 100%;
> +  height: 100%;
> +  padding: 0;
> +  background-color: #F9F9F9;

Instead of setting the background on the body, I suspect we want the newtab-scrollbox to be at 100%/100%. I think that would also fix some alignment issues - because I'm pretty sure that the current about:newtab places the search input / tiles slightly lower than with your patch applied.


However, it seems kinda silly to add so much nesting. Have you tried assigning the body the newtab-scrollbox id and styles?

::: browser/base/content/newtab/newTab.xhtml
@@ +8,2 @@
>    <!ENTITY % newTabDTD SYSTEM "chrome://browser/locale/newTab.dtd">
> +   %newTabDTD;

Let's keep the indentation on these %FooDTD;'s the way they were.

@@ +21,3 @@
>  
> +  <link rel="stylesheet" type="text/css" media="all"
> +  href="chrome://global/skin/" />

Generally speaking, we like to format the attributes of our tags so that the first ones on each line line up with one another.

Example: https://pastebin.mozilla.org/8836668

Applies to a bunch of places in this file.

        Attachment #8620951 -
        Flags: review?(mconley) → review-

    
  

        Attachment #8620951 -
        Attachment is obsolete: true

    
  

        Attachment #8620496 -
        Attachment is obsolete: true

    
  
Blocks: 1182625

    
    

    
  


  
Bug 1167601 - Convert newTab.xul to newTab.xhtml v2

    
    

    
  


  
Bug 1021654 - Convert about:newtab to content process for e10s

        Attachment #8634343 -
        Flags: review?(mconley)

    
    

    
  
I think the XUL -> XHTML patch should be good to go, now that the one-off search stuff has landed.

Likely this patch needs to be rebased, and we'll need to make sure that the one-off search stuff still works and the tests all pass, and then we can go ahead and land this.

    
  

        Attachment #8634343 -
        Attachment is obsolete: true

        Attachment #8634343 -
        Flags: review?(mconley)

    
  

        Attachment #8634342 -
        Flags: review?(mconley)

    
    

    
  
Comment on attachment 8634342 [details]
MozReview Request: Bug 1167601 - Convert newTab.xul to newTab.xhtml v2

Bug 1167601 - Convert newTab.xul to newTab.xhtml v2

    
    

    
  
Comment on attachment 8634342 [details]
MozReview Request: Bug 1167601 - Convert newTab.xul to newTab.xhtml v2

https://reviewboard.mozilla.org/r/13379/#review12443

Alright, let's land this puppy! Great work!

        Attachment #8634342 -
        Flags: review?(mconley) → review+

    
    

    
  
url:        https://hg.mozilla.org/integration/fx-team/rev/ee371e7b0af09d6c2b9529f47f1f4c1495c4b5f1
changeset:  ee371e7b0af09d6c2b9529f47f1f4c1495c4b5f1
user:       Ursula <usarracini@mozilla.com>
date:       Mon Jun 15 10:23:33 2015 -0400
description:
Bug 1167601 - Convert newTab.xul to newTab.xhtml. r=mconley

    
    

    
  
url:        https://hg.mozilla.org/integration/fx-team/rev/7c03f6f2656b113934fa1b75d0f9fe00d097a83b
changeset:  7c03f6f2656b113934fa1b75d0f9fe00d097a83b
user:       Mike Conley <mconley@mozilla.com>
date:       Wed Jul 22 16:46:26 2015 -0400
description:
Bug 1167601 - Follow-up to fix an a11y test that assumed that about:newtab was still XUL. r=tbsaunde

    
    

    
  
Note that tbsaunde reviewed that patch over IRC.

    
    

    
  
This landed on central. For some reason, it didn't get closed.

https://hg.mozilla.org/mozilla-central/rev/ee371e7b0af0
https://hg.mozilla.org/mozilla-central/rev/7c03f6f2656b
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42

    
    

    
  
Marking as addon-compat because some addons (like ours) might be doing interesting things with the new tab page and this change removes the ability to overlay it.
Keywords: addon-compat
Depends on: 1206692
Depends on: 1199425

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: